-
Notifications
You must be signed in to change notification settings - Fork 4.5k
Allow Dataform Select Box to have a custom empty option #70894
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
Allow Dataform Select Box to have a custom empty option #70894
Conversation
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
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.
Nice touch, thanks!
* - in bulk editing | ||
* | ||
*/ | ||
{ label: __( 'Select item' ), value: '' }, |
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.
What if we customized the default empty state by using the existing field.label
? If the field's label is "Date", the empty item could be "Select date" (lowercase and without an article to avoid grammar mistakes).
Providing your own custom element would be a minor use case (fine to still support it).
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.
yes that could also work but there may be issues with casing?.
Do you want me to go ahead with adding Select %s
, or just merge this as is?
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.
I think using sentence case here would be fine, at least I can't come up with an example where lowercasing the field's label would be problematic. In any case, this would be just the default. The field authors would be able to override it by providing its own empty state, if it was wrong – so that'd be fine.
Given is a small change, I think I'd do it in this PR to have a better default.
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.
@oandregal Thinking about it some more I think it might be a worse default. There is no guarantee the label will be the noun. For a simplistic label like "Country", it would work as "Select country" but labels could be sentences. For example I could label my input "Select a country". That would in term make the default "Select select a country". I think this is too unpredictable. We should keep the generic term and leave the default in the hands of the consumer.
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.
All the labels I've seen in practice are a noun and, after this PR, there's the ability to override, so I don't think it'd be an issue.
It's fine if you'd rather ship this PR as it is, we can revisit this idea in the future should it come to it.
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.
Yup lets do that 👍🏻 Thanks
Can we add a changelog entry? |
What?
Updates the select component under the dataviews package.
Currently a value of "Select Item" is forced before any provided option values. if you attempt to pass your own blank option, for example, with improved wording, you get a duplicate:
This PR changes this behaviour to first check if a blank option is provided as an element before rendering "select item".
Why?
Consumers should have control over the "Select X" option.
How?
Checks if an empty option is already provided by the consumer.
Alternative would have been extra props but this seemed like a more logical approach.
Testing Instructions
Testing Instructions for Keyboard