fix: support "disabled" state in MultiSelect #1377
Conversation
Prevent user interaction when MultiSelect is disabled and update UI to hint at disabled state
|
@MeoMix is attempting to deploy a commit to the Themesberg Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThe Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant MultiSelect
User->>MultiSelect: Attempt interaction (click/add/remove)
alt MultiSelect is disabled
MultiSelect-->>User: No action taken
else MultiSelect is enabled
MultiSelect-->>User: Interaction proceeds as normal
end
Assessment against linked issues
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
| @@ -21,7 +22,7 @@ | |||
| }; | |||
|
|
|||
| // Container | |||
| const multiSelectClass: string = 'relative border border-gray-300 flex items-center rounded-lg gap-2 dark:border-gray-600 focus-within:ring-1 focus-within:border-primary-500 ring-primary-500 dark:focus-within:border-primary-500 dark:ring-primary-500 focus-visible:outline-none'; | |||
| const multiSelectClass: string = 'relative border border-gray-300 flex items-center rounded-lg gap-2 dark:border-gray-600 ring-primary-500 dark:ring-primary-500 focus-visible:outline-none'; | |||
There was a problem hiding this comment.
Here I ensure that clicking the input doesn't highlight it when focused. Visually this is good, but conceptually I don't think any of the children should be retaining focus. I think it would be better to try and propagate the disabled state through all the children for accessibility, but I didn't attempt to do that.
| {#if !selectItems.length} | ||
| <span class="text-gray-400">{placeholder}</span> | ||
| {/if} | ||
| <span class="flex gap-2 flex-wrap"> | ||
| {#if selectItems.length} | ||
| {#each selectItems as item (item.name)} | ||
| <slot {item} clear={() => clearThisOption(item)}> | ||
| <Badge color="dark" large={size === 'lg'} dismissable params={{ duration: 100 }} on:close={() => clearThisOption(item)}> | ||
| <Badge color="dark" large={size === 'lg'} dismissable params={{ duration: 100 }} on:close={() => clearThisOption(item)} class={disabled && "pointer-events-none"} > |
There was a problem hiding this comment.
It's probably overkill to do pointer-events: none here since all helper methods have early exits, but meh.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- src/lib/forms/MultiSelect.svelte (10 hunks)
Additional comments not posted (7)
src/lib/forms/MultiSelect.svelte (7)
14-14: Newdisabledproperty added.The addition of a
disabledproperty is essential for implementing the disabled state functionality. This change is well-implemented and follows Svelte's reactive declaration pattern.
42-44: Ensure all interaction handlers respect thedisabledstate.All key interaction handlers (selecting options, clearing options, keyboard navigation) correctly check the
disabledproperty before proceeding. This ensures that no actions are performed when the component is disabled.Also applies to: 55-57, 65-67, 77-79, 86-88, 99-101, 117-119
151-151: UI state handling for disabled MultiSelect.The div handling clicks and focus-out events correctly checks the
disabledstate before toggling visibility. This prevents any interaction when the component is disabled, which is crucial for accessibility and user experience.
159-159: Badge component handling in disabled state.The Badge component within the slot correctly adds a
pointer-events-noneclass when the component is disabled. This is a good use of conditional rendering to ensure that UI elements inside the MultiSelect do not respond to user interactions when disabled.
[APROVED]
168-168: Close button disabled state handling.The CloseButton component is appropriately disabled when the MultiSelect is disabled. This is done through both a
disabledattribute and conditional class application, which is a robust approach to ensure the button is non-interactive.
171-171: Dropdown arrow handling in disabled state.The SVG for the dropdown arrow correctly applies the
cursor-not-allowedstyle when the component is disabled. This visual cue is important for indicating to the user that the component is non-interactive.
180-180: Conditional rendering for item selection in disabled state.The div that allows item selection within the dropdown applies
pointer-events-nonewhen the component is disabled. This prevents any selection actions, aligning with the intended functionality of the disabled state.
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Thank you for your contribution. Your function is applied but not from this PR. https://flowbite-svelte.com/docs/forms/select#Disabled_option |
Closes #1349
📑 Description
Currently, MultiSelect remains receptive to user input even if disabled=true is passed
This PR minimally addresses that issue by introducing disabled state and adjusting the UI to accommodate. The cursor changes to "not-allowed", elements gray out, and user input is denied.
I tried to change as little as possible and just get in/out with a laser fix. I'm not especially interested in putting a lot of time into this project (sorry!) just trying to be kind and upstream a patch for a bug I encountered.
Status
✅ Checks
If someone would like to go through these checks - by all means. I tested locally on my own project by copy/pasting the existing component into my codebase and it works on my machine.
ℹ Additional Information
Summary by CodeRabbit
disabledproperty to theMultiSelectcomponent, allowing users to control the component's interactivity. When set totrue, the component will be non-interactive.