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

Add support for disabled drop down items in SelectControl #15976

Conversation

philipjohn
Copy link
Contributor

Description

Fixes #11270. Adds support for marking options within a SelectControl as 'disabled', which translates to the disabled attribute being added to the HTML option element.

How Has This Been Tested?

  1. Added the Tag Cloud block to a new post
  2. Observed that the "Taxonomy" sidebar setting included a "- Select -" option that could be selected.
  3. Applied the patch and ran the build
  4. Repeated step 1
  5. Observed that the "- Select -" option is now disabled

Screenshots (jpeg or gifs if applicable):

Before:
tag-cloud-before

After:
tag-cloud-after

Types of changes

This changes improves an existing component and updates one use of that component to demonstrate how it can be used.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows has proper inline documentation.

…ectControl` as 'disabled', which translates to the `disabled` attribute being added to the HTML `option` element.
@gziolo gziolo added First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository [Type] Enhancement A suggestion for improvement. [Feature] UI Components Impacts or related to the UI component system [Package] Components /packages/components Good First Review A PR that's suitable for someone looking to contribute for the first time by reviewing code Needs Accessibility Feedback Need input from accessibility labels Jun 4, 2019
@afercia
Copy link
Contributor

afercia commented Jun 14, 2019

Discussed during today's accessibility bug scrub. Agreed we don't see any accessibility issue in implementing an <option disabled>...</option>. Are we missing something perhaps? Please do let us know if a specific accessibility feedback is needed here.

@afercia afercia added [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). and removed Needs Accessibility Feedback Need input from accessibility labels Jun 14, 2019
Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

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

It's not clear to me: What's the purpose of the "- Select -" option existing at all? My initial worry was that the block allows for the value to be "unset", which we'd regress on with the changes here. But the behavior from choosing the value seems to be invalid (displays a message "Your site has no tags"). Perhaps it's meant as a fallback condition for sites with no taxonomies registered?

@philipjohn
Copy link
Contributor Author

It's not clear to me: What's the purpose of the "- Select -" option existing at all?

It's to provide a default option as a placeholder, so that it doesn't look to the user as though the first option is selected - it hasn't, if you check the value of the select.

My initial worry was that the block allows for the value to be "unset"

This won't happen - the initial value is disabled for that very reason.

Perhaps it's meant as a fallback condition for sites with no taxonomies registered?

It's for anywhere that a dropdown is used and the list of option elements for that value is not pre-determined or completely known.

One way you can test this is to create a simple block with a drop down in the block controls. Give the drop down a single value. At the moment, the result would be that it looks to the user as though that single option is selected. If you use JS to get the value of the select, it will be empty (so not what the user sees). If the user attempts to select that single option, nothing actually happens - the value never changes.

@philipjohn
Copy link
Contributor Author

Are we missing something perhaps?

@gziolo Did you just want a gut check from the a11y team? Given there are no issues identified, how are we on getting this approved?

iuravic added a commit to Automattic/newspack-plugin that referenced this pull request Jul 21, 2019
Created a local copy of the SelectComponent from @wordpress/components which implements the change proposed in
WordPress/gutenberg#15976. The Newspack's component now uses the local
assets/components/src/select-control-gutenberg-modified. Once the PR is implemented, we should switch back to
the WP component.

Added the first disabled option in the Components Demo Wizard. Also updated the labels there.

Updated the SelectComponent's CSS, improved the disabled SelectOption visuals.
Copy link
Contributor

@brentswisher brentswisher left a comment

Choose a reason for hiding this comment

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

This looks good, thanks fo the work @philipjohn. I have used the pattern you described of a disabled first item in other projects and it was very useful, and looks like a good fit for the tag cloud here.

@brentswisher
Copy link
Contributor

@aduth @afercia this looks good to merge to me unless you have any objections?

@aduth
Copy link
Member

aduth commented Jul 30, 2019

No objections from me.

@afercia
Copy link
Contributor

afercia commented Jul 31, 2019

Thanks for working on this! No objections from me in having the ability to disable an option.

Not sure about the specific usage on the Tag Cloud taxonomy select. One of the taxonomies must be always selected, so I'm not sure I understand what is the purpose of the - Select - option. I'd rather remove it. Also, the legacy Tag Cloud widget doesn't have a - Select - option.

@philipjohn
Copy link
Contributor Author

@afercia In the absence of a default, the browser may select the first option against the user expectation. You can see an example in Automattic/newspack-plugin#83

This could also affect the tag cloud, hence why the default disabled option is included here.

@brentswisher
Copy link
Contributor

@afercia thanks for the feedback, since the - Select - is already there in the "new" tag cloud and this just makes it disabled I would say it's still a step in the right direction. Could explore dropping it in a different issue, but don't see that as a blocker to this. Thanks again @philipjohn

@brentswisher brentswisher merged commit 7d20066 into WordPress:master Jul 31, 2019
@brentswisher brentswisher added this to the Gutenberg 6.3 milestone Jul 31, 2019
@afercia
Copy link
Contributor

afercia commented Jul 31, 2019

see an example in Automattic/newspack-plugin#83

That sounds like a bug in the code, which is assuming a state change 🙂 Native selects don't work that way. I'd say the component fails in reproducing the native, expected, behavior.

  • the selected option misses a selected attribute
  • the selected attribute is specifically meant to indicate whether the option is selected by default
  • browsers will understand what the initially selected option is thanks to the selected attribute
  • the code should take into account the selected attribute and update it as necessary

Will create a new issue...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] UI Components Impacts or related to the UI component system First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). Good First Review A PR that's suitable for someone looking to contribute for the first time by reviewing code [Package] Components /packages/components [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature request: Support "disabled" attribute on options in SelectControl.
5 participants