feat(dropdown-select): add disabled state for individual options #2174
Conversation
✅ Deploy Preview for marvelous-moxie-a6e2fe ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
hi @dadadcko, thanks for opening a PR! We document the components in the "storybook-vue" folder for the relative component (in this case storybook-vue/stories/components/dropdown-select) - but I am not sure we need it for this case, as the disabled prop is already listed there (admittedly for the whole dropdown-select component, and not the dropdown-select-item) - we could either add another table to document dropdown-select-item props (like we do in the radio-button-group documentation, for individual radio props), or add another story to show how this prop works. I would go with the first option, as the stories displayed are usually decided internally by the scale team, involving also designers/content editors etc. |
… disabled select items feature Documentation is located under `dropdown-select` component section, since `dropdown-select-item` is its child relative. Added both stories for component and new disabled features as well as updated usage docs with disabled state of dropdown-select.
…on first action On first keyboard action after opening box with items, if the action is Previous, it should still focus first enabled element (if exists). It worked like this before, so fixed position calculation for this action when this edge-case happens
|
Hey @felix-ico, I have added docs for the new feature in the way you suggested. Please double check docs, mainly German part, since I don't speak German very well and just |
|
Hi @dadadcko, I didn't look into the code in detail yet, but one thing that stands out to me is that there is no visual indication that the disabled items can not be selected. I think this is more of a UX/UI matter though so I would kindly ask @JoschaIco to have a look at the preview here https://deploy-preview-2174--marvelous-moxie-a6e2fe.netlify.app/?path=/docs/components-dropdown-select--disabled-items to clarify. We should also test this for accessibility (can visually impaired people recognise disabled dropdown-select-items via screen reader), i can take care of this. Apart from that all the CI checks seem to pass so in theory the PR could be merged, if all the above points are taken care of. Thanks again for the effort :) |
|
@felix-ico Thanks for the checks! Regarding accessibility features, disabled items are skipped while navigating with keyboard (taken from native html select tag). Also, the latest error in visual-tests CI pipeline seems like not related to my PR, it seems to be something with Glad to help, if there is anything more I could do, hit me up! Also, if we have more time in team sprints, I might look at other issues too. :) |
ah perfect than that is already taken care of
this is a known issue, that visual test is flakey, so no worries - I will open a separate PR to skip that test because it pops up again and again in unrelated PRs |
|
Hi @dadadcko and @felix-ico, I only got one tiny remark: disabled menu items should not have a hover state at all. In your implementation those disabled items get a grey background on hover and the mouse cursor is changing. It would correct if only the mouse cursor would change and the disabled item would not show any change on hover.
@dadadcko could you please adjust that? Other than that it looks all fine to me 👍 |
|
@JoschaIco Sure will take care of it. Thanks for your input! Edit: Fixed by next commit |
|
@felix-ico Hey, just a reminder in case you've missed auto notifications. All comments have been resolved and all pipeline checks seems to pass. You can start freely now with code review any time you want :) |
|
hi @dadadcko i just left one comment the rest looks good to me, thanks! |
|
Hello @felix-ico I see your comment that you did code review and you said you have left 1 comment there, but I cannot see it. Could you please double-check if you added it and if I have permissions to see it ? |
packages/components/src/components/dropdown-select/dropdown-select.css
Outdated
Show resolved
Hide resolved
|
@dadadcko sorry apparently the review was still pending, can you see it now? |
@felix-ico Yes, it is visible now. I have your inputs in the latest commit. Other than that I think it is now ready, if you don't have anything else to add :) |
|
Thank you @dadadcko for this amazing PR 🙏 (@felix-ico thank you for reviewing!) |
This PR adds new feature for
dropdown-selectcomponent, as described in (#2114).Also added tests (both unit and e2e) for this feature.
Regarding disabled reasons, I came to decision not to implement it, since component already provides 3 slots, where consumer can define reason of some sort.
Disabled state is supported for any elements with
disabledattribute, not only forscale-dropdown-select-itemcomponent.Keyboard navigation handlers was adjusted to skip any disabled attributes (similar to native
selectelement).@acstll @felix-ico
Question for maintainers: Should I also add some entries to Docs? If so, can you give me a hint, where to add it exactly?
fixes #2114