-
Notifications
You must be signed in to change notification settings - Fork 333
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
FIX: put current page at top of ClassName dropdown (was broken) #3004
Conversation
The "Issues" section is for you to link to an issue. This is made clear in the pull request template:
Please create a GitHub issue if there isn't one already and link to it. If there's already an issue, please link to that. |
You've also put "[ N]" next to "This change is covered with tests (or tests aren't necessary for this change)" Does this mean you think tests are needed, but haven't included them? |
Test are nice, but I dont have time for it and I dont think such a simple piece of code needs a test (famous last words), partially because it has not had a test so far, AFAIK. |
In Terms of a test, I think it would be more valuable for a human to see that the UX feels good and works nicely. I was thinking that you could have:
etc... |
The point of unit tests is primarily to prevent regressions. This is a perfect example of something that was working at some point, but due to lack of unit tests it broke at some stage without anyone noticing. To prevent this breaking again, it would be best to have a unit test. |
What exactly would you test for? Would you test that it returns an array or that it returns a valid array? @GuySartorelli - I'd be curious if you had any ideas on what exactly to test for. |
bb507e5
to
2735149
Compare
I have added a bunch of tests. |
Just passing by, left a few minor comments. Thanks for taking the time to add the tests as well, @sunnysideup! |
6029bf7
to
d8748ff
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM and works well locally.
Thank you for making this change.
/** | ||
* @return void | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/** | |
* @return void | |
*/ |
We don't need this typehint, but I won't delay merging just for this.
Description
For the "Page Type" field in the settings tab (main) of a page, the current Page Type is supposed to be on top (see code changed). The code did not actually do that though and instead just placed the "last" entry in the list on top. That does not make any sense so I fixed it.
Manual testing steps
Open any SS website, go to the settings of a page and look at the list of Page Type options. You will see they are in alphabetical order, but the last one (e.g. the one that starts with last letter in the alphabet is listed first). The current page type is supposed to be listed first!
Issues
#3005
Also see:
#2209
#2208
Pull request checklist