feat: multi-input form-component #1549
Conversation
|
@superstes is attempting to deploy a commit to the Themesberg Team on Vercel. A member of the Team first needs to authorize it. |
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughA new Svelte component, Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant MultiInput
participant Badge
participant CloseButton
User->>MultiInput: Enter input (using defined separators)
MultiInput->>MultiInput: Process input & update internal state
MultiInput->>Badge: Create/display badge for each entry
alt Clearable enabled
User->>CloseButton: Click clear icon for a value
CloseButton->>MultiInput: Trigger clearing of specific value
else Clear all values
User->>MultiInput: Activate clear all
MultiInput->>MultiInput: Remove all input items
end
MultiInput->>User: Dispatch update event
Possibly related PRs
Poem
🪧 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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
src/lib/forms/MultiInput.svelte (6)
1-3: Add validation for edge cases in clampSize function.The function currently handles mapping "xs" to "sm" and "xl" to "lg", but it doesn't validate if
sis in the set of allowed sizes.<script context="module">export function clampSize(s) { - return s && s === "xs" ? "sm" : s === "xl" ? "lg" : s; + const allowedSizes = ["sm", "md", "lg"]; + if (!s) return "md"; + if (s === "xs") return "sm"; + if (s === "xl") return "lg"; + return allowedSizes.includes(s) ? s : "md"; } </script>
59-71: Improve separator handling and event dispatching.The current implementation doesn't handle multi-character separators and doesn't dispatch a change event after adding a new value.
function handleKeyDown(event) { if (disabled) return; - let seps = separators.split('') + // Split by characters only if separators is a string + let seps = typeof separators === 'string' ? separators.split('') : separators; seps.push("Enter"); for (let s of seps) { if (event.key == s) { handleInputSeparation(); + dispatcher("change"); event.stopPropagation(); event.preventDefault(); return; } } };
107-109: Simplify array length check and avoid duplicate close buttons.The component has two close buttons: one inside the input area when there are items, and another outside when
clearableis true. This could be confusing to users.- {#if clearable && value && `${value}`.length > 0} - <CloseButton {size} on:click={clearAll} color="none" class=" {twMerge(floatClass, classRight)} focus:ring-0 end-6 focus:ring-gray-400 dark:text-white" /> + {#if clearable && value && value.length > 0 && !inputItems.length} + <CloseButton {size} on:click={clearAll} color="none" class="{twMerge(floatClass, classRight)} focus:ring-0 end-6 focus:ring-gray-400 dark:text-white" /> {/if}
117-117: Documentation inconsistency.The default value for
valueis documented asundefined, but in the actual code it's an empty array ([]).-@prop export let value: $$Props['value'] = undefined; +@prop export let value: $$Props['value'] = [];
6-22: Enhance ARIA support for accessibility.The component has a
role="listbox"attribute but doesn't fully implement the required ARIA attributes for a listbox interface. Consider enhancing accessibility by adding appropriate ARIA attributes.The component should implement additional ARIA attributes like
aria-label,aria-multiselectable, and each item should haverole="option"with appropriate state attributes. Here's how you might approach it:
- Add an
aria-labelprop to the component:+export let ariaLabel = "Multi-input selection";
- Then update the listbox div to include these attributes:
- <div tabindex="0" role="listbox" class={twMerge(multiInputClass, sizes[size], $$props.class, !disabled && "focus-within:ring-1 focus-within:border-primary-500 dark:focus-within:border-primary-500", disabled && "opacity-50 cursor-not-allowed")}> + <div tabindex="0" role="listbox" aria-label={ariaLabel} aria-multiselectable="true" class={twMerge(multiInputClass, sizes[size], $$props.class, !disabled && "focus-within:ring-1 focus-within:border-primary-500 dark:focus-within:border-primary-500", disabled && "opacity-50 cursor-not-allowed")}>
- Update the Badge elements to include appropriate ARIA roles:
- <Badge color="dark" large={size === 'lg'} dismissable params={{ duration: 100 }} on:close={() => clearThisOption(item.value)} class={disabled ? "pointer-events-none" : undefined} > + <Badge color="dark" large={size === 'lg'} dismissable params={{ duration: 100 }} on:close={() => clearThisOption(item.value)} class={disabled ? "pointer-events-none" : undefined} role="option" aria-selected="true" >
41-45: Consider making the clearAll function a public method.Since this is a core function of the component, consider exposing it as a public method so users can programmatically clear the input.
-const clearAll = (e) => { +export function clearAll(e) { + if (e) { e.stopPropagation(); + } value = []; dispatcher("change"); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/lib/forms/MultiInput.svelte(1 hunks)src/lib/index.js(1 hunks)
🔇 Additional comments (1)
src/lib/index.js (1)
89-89: MultiInput component export looks good!The export follows the established pattern in the codebase, and the component is correctly placed in the Forms section with the other form components.
…ixed item-ids, handle empty inputs
|
@coderabbitai review |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 14479 @superstes I've found the issue. In MultiInput.svelte, even when color is set to "red", the focus ring stays blue (primary-500) instead of turning red like in Input.svelte. The problem is in the CSS class calculation for multiInputClass. There are hardcoded focus-within styles that override the color-specific focus styles: multiInputClass = twMerge(
containerDefaultClass,
sizes[size],
$$props.class,
!disabled && 'focus-within:ring-1 focus-within:border-primary-500 dark:focus-within:border-primary-500', // This line forces primary color
disabled && 'opacity-50 cursor-not-allowed',
ringClasses[color], // This is correct but gets overridden
borderClasses[_color],
colorClasses[_color]
);To fix this, you should remove the hardcoded primary-500 color and just keep the focus-within:ring-1 class: multiInputClass = twMerge(
containerDefaultClass,
sizes[size],
$$props.class,
!disabled && 'focus-within:ring-1', // Remove hardcoded primary-500 colors
disabled && 'opacity-50 cursor-not-allowed',
ringClasses[color], // This will now control the ring color properly
borderClasses[_color],
colorClasses[_color]
);This way, when color="red", the ring will correctly use the red focus styles defined in ringClasses.red. (_/) |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 14591 @superstes I found the issue. In the !disabled && 'focus-within:ring-1 focus-within:border-primary-500 dark:focus-within:border-primary-500'To fix this, you need to make the focus ring color dynamic based on the <script>
// Add this new object after the existing ringClasses
const focusRingClasses = {
base: 'focus-within:ring-1 focus-within:border-primary-500 dark:focus-within:border-primary-500',
green: 'focus-within:ring-1 focus-within:border-green-500 dark:focus-within:border-green-500',
red: 'focus-within:ring-1 focus-within:border-red-500 dark:focus-within:border-red-500'
};
// Then modify your reactive statement to use this object
$: {
const _color = color === 'base' && background ? 'tinted' : color;
inputClass = twMerge([inputDefaultClass, textSizes[_size], group || 'rounded-lg', group && 'first:rounded-s-lg last:rounded-e-lg', group && '[&:not(:first-child)]:-ms-px', $$props.class], colorClasses[_color]) + ' border-none rounded-none p-0';
multiInputClass = twMerge(containerDefaultClass, sizes[size], $$props.class, !disabled && focusRingClasses[color], disabled && 'opacity-50 cursor-not-allowed', ringClasses[color], borderClasses[_color], colorClasses[_color]);
}
</script>This will ensure that when |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@superstes I found the issue. The
export let inputInvalid = false;
$: colorClass = inputInvalid ? 'red' : color;
<Wrapper
class={wrapperClass}
{size}
{floatClass}
{color}
{inputInvalid}
>This mirrors how 🐰 looking thoughtfully at the code 🏁 Script executed: Length of output: 9009 🏁 Script executed: Length of output: 5469 @superstes After examining both components, I've found the issue. In
Here's how to fix it:
export let inputInvalid = false;
$: {
const _color = inputInvalid ? 'red' : (color === 'base' && background ? 'tinted' : color);
// Rest of the code remains the same
}
<Wrapper
class={wrapperClass}
show={$$slots.left || $$slots.right}
{inputInvalid}>This should ensure that the ring properly turns red when the input is invalid, matching the behavior in Input.svelte. 🐰 carefully examining the styles ✏️ Learnings added
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@superstes Good question about $: {
const _color = color === 'base' && background ? 'tinted' : color;
inputClass = twMerge([inputDefaultClass, textSizes[_size], group || 'rounded-lg', group && 'first:rounded-s-lg last:rounded-e-lg', group && '[&:not(:first-child)]:-ms-px', $$props.class], colorClasses[_color]) + ' border-none rounded-none p-0';
multiInputClass = twMerge(containerDefaultClass, sizes[size], $$props.class, !disabled && 'focus-within:ring-1 focus-within:border-primary-500 dark:focus-within:border-primary-500', disabled && 'opacity-50 cursor-not-allowed', ringClasses[color], borderClasses[_color], colorClasses[_color]);
}This appears to be inconsistent. In the Input component, $: {
const _color = inputInvalid ? 'red' : color === 'base' && background ? 'tinted' : color;
inputClass = twMerge([inputDefaultClass, colorClasses[_color], textSizes[_size], group || 'rounded-lg', group && 'first:rounded-s-lg last:rounded-e-lg', group && '[&:not(:first-child)]:-ms-2', $$props.class]);
}For consistency with the rest of the codebase, you should change 🏁 Script executed: Length of output: 14479 @superstes Good catch! This is indeed an inconsistency. Looking at the MultiInput component: $: {
const _color = color === 'base' && background ? 'tinted' : color;
inputClass = twMerge([...], colorClasses[_color]) + '...';
multiInputClass = twMerge(
...,
ringClasses[color], // Using color directly
borderClasses[_color], // Using modified _color
colorClasses[_color] // Using modified _color
);
}This inconsistency appears in Input.svelte too, where There's no obvious reason for this inconsistency. Since I recommend changing 🧠 Learnings used |
|
@coderabbitai The |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@superstes I see the issue now. The problem is in the hardcoded focus-within classes in the !disabled && 'focus-within:ring-1 focus-within:border-primary-500 dark:focus-within:border-primary-500'The focus-within border is always set to primary-500 (blue) regardless of the @superstes To fix the focus ring color issue when
const focusWithinClasses = {
base: 'focus-within:border-primary-500 dark:focus-within:border-primary-500',
primary: 'focus-within:border-primary-500 dark:focus-within:border-primary-500',
blue: 'focus-within:border-blue-500 dark:focus-within:border-blue-500',
green: 'focus-within:border-green-500 dark:focus-within:border-green-500',
red: 'focus-within:border-red-500 dark:focus-within:border-red-500',
yellow: 'focus-within:border-yellow-500 dark:focus-within:border-yellow-500',
purple: 'focus-within:border-purple-500 dark:focus-within:border-purple-500',
dark: 'focus-within:border-gray-500 dark:focus-within:border-gray-500'
};
$: {
const _color = inputInvalid ? 'red' : (color === 'base' && background ? 'tinted' : color);
inputClass = twMerge([inputDefaultClass, textSizes[_size], group || 'rounded-lg', group && 'first:rounded-s-lg last:rounded-e-lg', group && '[&:not(:first-child)]:-ms-px', $$props.class], colorClasses[_color]) + ' border-none rounded-none p-0';
multiInputClass = twMerge(
containerDefaultClass,
sizes[size],
$$props.class,
!disabled && 'focus-within:ring-1 ' + focusWithinClasses[_color], // Use dynamic focus colors
disabled && 'opacity-50 cursor-not-allowed',
ringClasses[_color],
borderClasses[_color],
colorClasses[_color]
);
}This way, when 🏁 Script executed: Length of output: 14766 🏁 Script executed: Length of output: 611 @superstes I see the problem now. The focus-within styling is still not dynamic, even though you've removed the hardcoded primary-500 border color. The issue is that you have: !disabled && 'focus-within:ring-1'This adds the ring styling but doesn't specify the color. Meanwhile, the Let's fix this by creating a new focus-within styles object: const focusWithinClasses = {
base: 'focus-within:border-primary-500 dark:focus-within:border-primary-500',
primary: 'focus-within:border-primary-500 dark:focus-within:border-primary-500',
green: 'focus-within:border-green-500 dark:focus-within:border-green-500',
red: 'focus-within:border-red-500 dark:focus-within:border-red-500'
};Then modify your reactive statement: $: {
const _color = inputInvalid ? 'red' : (color === 'base' && background ? 'tinted' : color);
const _colorRing = inputInvalid ? 'red' : color;
inputClass = twMerge([inputDefaultClass, textSizes[_size], group || 'rounded-lg', group && 'first:rounded-s-lg last:rounded-e-lg', group && '[&:not(:first-child)]:-ms-px', $$props.class], colorClasses[_color]) + ' border-none rounded-none p-0';
multiInputClass = twMerge(
containerDefaultClass,
sizes[_size],
$$props.class,
!disabled && 'focus-within:ring-1 ' + focusWithinClasses[_colorRing], // Use dynamic focus-within color
disabled && 'opacity-50 cursor-not-allowed',
ringClasses[_colorRing],
borderClasses[_color],
colorClasses[_color]
);
}This ensures that both the regular ring color (for direct focus) and the focus-within ring color (for when a child element has focus) will be dynamically set based on the inputInvalid state. ✏️ Learnings added
|
|
@coderabbitai Now it works when we click inside the div with |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@superstes I see the issue. The problem is that your Looking at your implementation, Here's how to fix it: // Add focus classes for the input
const inputFocusClasses = {
base: 'focus:ring-primary-500 focus:border-primary-500 dark:focus:border-primary-500',
primary: 'focus:ring-primary-500 focus:border-primary-500 dark:focus:border-primary-500',
green: 'focus:ring-green-500 focus:border-green-500 dark:focus:border-green-500',
red: 'focus:ring-red-500 focus:border-red-500 dark:focus:border-red-500'
};
// Update the reactive statement
$: {
const _color = inputInvalid ? 'red' : (color === 'base' && background ? 'tinted' : color);
const _colorRing = inputInvalid ? 'red' : color;
// Add the dynamic focus classes to inputClass
inputClass = twMerge(
[inputDefaultClass, textSizes[_size], group || 'rounded-lg',
group && 'first:rounded-s-lg last:rounded-e-lg',
group && '[&:not(:first-child)]:-ms-px',
$$props.class, inputFocusClasses[_colorRing]], // Add focus classes here
colorClasses[_color]
) + ' border-none rounded-none p-0';
// Keep your existing multiInputClass
multiInputClass = ...
}This way, both the container div and the input element itself will have the appropriate focus styling based on validation state. 🏁 Script executed: Length of output: 14582 @superstes I found the issue! When clicking directly on the input element, the container div gets the The problem is in your CSS classes: // This only adds focus-within:ring-1 without any color
!disabled && 'focus-within:ring-1',
// These are :focus styles, not :focus-within styles
ringClasses[_colorRing],To fix this, create a dedicated object for focus-within styles: // Add this after your other class definitions
const focusWithinClasses = {
base: 'focus-within:ring-1 focus-within:border-primary-500 dark:focus-within:border-primary-500',
primary: 'focus-within:ring-1 focus-within:border-primary-500 dark:focus-within:border-primary-500',
green: 'focus-within:ring-1 focus-within:border-green-500 dark:focus-within:border-green-500',
red: 'focus-within:ring-1 focus-within:border-red-500 dark:focus-within:border-red-500'
};
// Then update the reactive statement
$: {
const _color = inputInvalid ? 'red' : (color === 'base' && background ? 'tinted' : color);
const _colorRing = inputInvalid ? 'red' : color;
inputClass = twMerge([...], colorClasses[_color]) + ' border-none rounded-none p-0';
multiInputClass = twMerge(
containerDefaultClass,
sizes[_size],
$$props.class,
!disabled && focusWithinClasses[_colorRing], // Use dynamic focus-within styles
disabled && 'opacity-50 cursor-not-allowed',
ringClasses[_colorRing],
borderClasses[_color],
colorClasses[_color]
);
}This will ensure the correct color is applied to the focus ring regardless of whether you click on the container or directly on the input element. ✏️ Learnings added
|
|
Focus color seems to be fixed now |
|
Is it possible to adopt flowbite-svelte@v1? |
📑 Description
I've seen the MultiSelect form-component in the docs.
For a project I need to have a MultiInput.
This is an implementation-draft for such a component.
I've basically copied the Input component and combined it with the MultiSelect.
I'm no svelte-pro - so you might be able to optimize it.
Status
✅ Checks
ℹ Additional Information
Initially it looks the same as an
InputcomponentThe user can type in it..
By pressing
Enteror,(the defaultseparators) the input gets saved to the array. Entries can be removed the same way as theMultiSelecthas implemented it. (single or all)The validation-callback can be used to validate inputs before they get saved to the array.
Example: The callback checks if the input is a valid IP-address and returns true/false.
Before 'commit'
After (
validationErrorClass)Summary by CodeRabbit