Show comma separated list in ready only mode of select tree control #38052
Conversation
github-actions
bot
added
the
package: @woocommerce/components
|
Hi , @woocommerce/mothra Apart from reviewing the code changes, please make sure to review the testing instructions as well. You can follow this guide to find out what good testing instructions should look like: |
Test Results SummaryCommit SHA: d26e9d5
To view the full API test report, click here. To view the full E2E test report, click here. To view all test reports, visit the WooCommerce Test Reports Dashboard. |
| disabled, | ||
| inputProps = {}, | ||
| suffix = <SuffixIcon icon={ search } />, | ||
| suffix = <SuffixIcon icon={ chevronDown } />, |
There was a problem hiding this comment.
Now that it is a chevron it makes me want to click on the suffix in order to open the menu, but at the moment it seems to do nothing when I click on the suffix.
| } | ||
| ); | ||
|
|
||
| if ( isReadOnly ) { |
There was a problem hiding this comment.
Could we make this not the default? and allow us to have an additional prop that either enables the comma separated list, or more abstract and expose a call back where a component can render the items manually:
selectedItems: ( ...props ) => false | JSX.Element; // if false we render the normal?
There was a problem hiding this comment.
I mostly mention this, as we use this within Attributes as well, and the comma separated items looks weird when a placeholder is also set:
@jarekmorawski I don't know if your intention is to have this to be the new default for all multi-select dropdowns?
There was a problem hiding this comment.
Yes, although we may want to do another iteration based on user feedback. 👍 No need to show the placeholder text when the field is not focused and the dropdown is closed.
There was a problem hiding this comment.
Updated this to hide the placeholder when in read only mode. Kept the comma separation for all experimental select controls.
|
@louwie17 updated based on your and @jarekmorawski's feedback. This should be ready for re-review. |
louwie17
left a comment
louwie17
left a comment
There was a problem hiding this comment.
@joshuatf thanks for the updated changes, I did notice a couple more bugs now. This seems to be a trickier one.
In the attributes add modal, the single select seems to hide my selected item once it has been selected. And in the multi-select within the Modal I can't seem to remove the selected items when clicking on the x. (see video)
Kapture.2023-05-04.at.14.27.07.mp4
One last suggestion, do you think you can change the mouse over icon for the suffix as well? to the pointer?
|
Thanks for the review again, @louwie17! Updated one more time and it appears everything is working on my end, but let me know if you see otherwise. I think we have a lot of scope creep in this issue and there are underlying issues with how the
I don't think we should do this because the suffix could be any icon and the current behavior in the select control does not always toggle the menu open or closed. At this point, this component feels pretty inconsistent (even in |
louwie17
left a comment
louwie17
left a comment
There was a problem hiding this comment.
Thanks for the updates @joshuatf, this is testing very well on my end now as well 🚀
I think we have a lot of scope creep in this issue and there are underlying issues with how the SelectTree is currently structured. The duplication in this component and borrowing of classes makes me pretty nervous and I think we need to set some time aside to refactor this at some point.
Yeah I agree, coming back to refactor and do some cleanup is a great idea! These two components (the select control and the tree select control) have become two of our most complicated components despite the numerous refactors already 😢
I don't think we should do this because the suffix could be any icon and the current behavior in the select control does not always toggle the menu open or closed.
Sounds good!
At this point, this component feels pretty inconsistent (even in trunk). We probably need to sit down with @jarekmorawski at some point and consolidate some of our expectations around UI behavior so we can clean up the logic in these components.
👍 👍
Happy to do it! I'll make that happen. 👍 |
Submission Review Guidelines:
Changes proposed in this Pull Request:
Shows a comma separated list for selected values when a select control is in read-only mode (menu closed). Note that in order for this behavior to work and to allow the "remove" buttons on the tags to continue to work when the menu is open, we need to keep the menu open when clicking the input or clicking one of the remove buttons. Otherwise, clicking the "remove" button closes the menu and the list reverts to read only mode before the item can be removed.
cc @jarekmorawski if this is the expectation and @louwie17 since you last touched this component's open/close state in case you have any "gotchas" around opening/closing.
Closes #37727 .
How to test the changes in this Pull Request: