Skip to content
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

Change mode switcher to a select box #389

Merged
merged 4 commits into from
Apr 10, 2017
Merged

Change mode switcher to a select box #389

merged 4 commits into from
Apr 10, 2017

Conversation

jasmussen
Copy link
Contributor

This PR changes the mode switcher from a custom JS thing to be a common selectbox, although heavily styled. The benefit is that you can click/hold the select box and release it on the option you want, making it easier to switch. It would also be more flexible if a plugin wanted to, say, add Markdown as an option.

GIF:

apr-10-2017 16-17-15

This will need a review, especially the React bits.

@jasmussen jasmussen requested review from youknowriad and aduth April 10, 2017 14:19
this.state = {
opened: false
value: this.props.mode
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't want to duplicate the source of truth from state. We'll simply want the render logic to use the prop directly. I'll plan to refactor this afternoon.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks.

@aduth
Copy link
Member

aduth commented Apr 10, 2017

@jasmussen
Copy link
Contributor Author

Also failing on this accessibility lint rule:

My linter actually caught this, but when I tested onBlur it wasn't an instant change whereas onChange was. Perhaps I was missing something.

@aduth
Copy link
Member

aduth commented Apr 10, 2017

My linter actually caught this, but when I tested onBlur it wasn't an instant change whereas onChange was. Perhaps I was missing something.

Depending on what the lint rule is trying to suggest (in this case a less immediate feedback from toggling through options) we may consider simply bypassing or removing the rule altogether, or applying both an onChange and onBlur.

@aduth
Copy link
Member

aduth commented Apr 10, 2017

For now I've disabled the rule with reasoning left inline. I'll plan to create a separate "Accessibility Review" issue to discuss the case, as onBlur handling is at odds with a select which has immediate effect (unless we also wanted a confirm action).

background: white;
select {
background: transparent;
line-height: 1;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No linting to capture it, but we ought to be cautious of mixed spacing in SCSS files.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just wanted to let you know that I really appreciate this and very much taking notes. Thanks for pushing fixes too.

Nesting input to defeat specificity of general `.wp-admin select`
selector
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants