feat(select): added invalid state #3940
Conversation
|
Preview: https://patternfly-pr-3940.surge.sh CSS Size Report
A11y report: https://patternfly-pr-3940-coverage.surge.sh |
|
This looks great! Thanks for adding this! |
|
LGTM - although should there be some explanatory text somewhere too? How would a user know what is wrong exactly when they make a selection? @jgiardino can't remember if this was discussed in the original issue now |
Looking at all PF examples, it's the Form component example that shows the error helper text that you're describing, and not the form input components like form control and form select. None of our current form examples use the Select component, so it's worth asking if we should show an example like that. However, in my use case, this is used for filtering, so the normal Form layout doesn't work there. Instead of displaying the error text inline, we display it in a tooltip. Maybe this would be worth showing in the React example for Select? @mcarrano What would you expect here? |
There was a problem hiding this comment.
Thanks @mcoker . This looks good to me. But is the user able to choose any of the validation styles to apply here, error, warning, or success? Is this what you were expecting @jgiardino ?
|
@mcarrano the valid/warning/error helper text comes from the form component, so a user would place a form select, custom select, text input, textarea, etc in a form, and upon either form or field validation, the form component shows the error/helper text. Similarly, there are no uses of the form helper text in the form control component examples, either. But you do see it on the form component examples I believe there are a couple of components that have that same helper text built in (date picker is one), so it seems like a valid use case to allow/move the helper text to be used outside of the context of the form. I believe we discussed this at some point and talked about maybe making helper text an inline text variation to the alert component, if I recall? |
|
Interesting question about application of the helper text. I do see it applied in the React example for Form Select here: https://www.patternfly.org/v4/components/form-select#validated I guess that I didn't realize use of the helper text was restricted to only when components are used inside of a form. I don't think users would make that distinction. That said, I'm not sure if we need to add it for the more general case. @jgiardino can you say a bit more about your use case? Would you want to have helper text below the field here instead of relying on a tooltp? I will say that it's a bit unexpected to get error states returned by a select at all since it implies that we are giving users invalid options to select from. Under what circumstances do we do that? |
We're using it in a filter. We have a maximum number of filters that can be defined at one time, and so the error case happens when the user has reached this max. Although with @mcarrano's point above, maybe error isn't the right variant for this, and a warning is more appropriate?:
Here are examples of what we have now. Ideally, the tooltip would display for the toggle, but we haven't made that update yet. In the case of using Select to specify a filter, we keep selected options as enabled, and disable the non-selected options. This let's a user clear a filter from the menu but not select additional options.
For comparison, here is a filter option that is a text input. This is behavior that we'd like to duplicate for the Select component.
|
|
Thanks for that additional detail @jgiardino . Given what you are showing here, not sure if adding helper text below the field is the right approach. What do you think @mmenestr ? |
|
I think as long as there is some sort of explanation the user can have access to (in this case a tooltip) then this should be good to go! |
|
🎉 This PR is included in version 4.92.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
fixes #3874