Skip to content

Feat/radio input #176

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Jun 4, 2025
Merged

Feat/radio input #176

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
250dba1
feat: added a radio button custom
Sahil2004 May 29, 2025
f8720aa
docs: added name option in docs.
Sahil2004 May 29, 2025
dcdd7d4
fix: merge conflict.
Sahil2004 Jun 2, 2025
4249ed2
chore: cleaned the unnecessary classes and variables for input type r…
Sahil2004 Jun 3, 2025
504be58
fix: moved input radio to its own component.
Sahil2004 Jun 4, 2025
eb442b6
fix: keydown events added.
Sahil2004 Jun 4, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
74 changes: 41 additions & 33 deletions platforms/metagram/src/lib/ui/Input/Input.stories.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,54 +1,62 @@
import { Input } from '..';
import { Input } from "..";

export default {
title: 'UI/Input',
component: Input,
tags: ['autodocs'],
render: (args: { type: string; placeholder: string }) => ({
Component: Input,
props: args
})
title: "UI/Input",
component: Input,
tags: ["autodocs"],
render: (args: { type: string; placeholder: string }) => ({
Component: Input,
props: args,
}),
};

export const Text = {
args: {
type: 'text',
placeholder: 'Joe Biden'
}
args: {
type: "text",
placeholder: "Joe Biden",
},
};

export const Tel = {
args: {
type: 'tel',
placeholder: '987654321'
}
args: {
type: "tel",
placeholder: "987654321",
},
};

export const NumberInput = {
args: {
type: 'number',
placeholder: 'Enter something'
}
args: {
type: "number",
placeholder: "Enter something",
},
};

export const Email = {
args: {
type: 'email',
placeholder: '[email protected]'
}
args: {
type: "email",
placeholder: "[email protected]",
},
};

export const Invalid = {
args: {
type: 'email',
placeholder: 'Invalid email',
value: 'not-an-email'
}
args: {
type: "email",
placeholder: "Invalid email",
value: "not-an-email",
},
};

export const Password = {
args: {
type: 'password',
placeholder: 'Please enter password'
}
args: {
type: "password",
placeholder: "Please enter password",
},
};

export const Radio = {
args: {
type: "radio",
value: "option1",
name: "option-1",
},
};
68 changes: 56 additions & 12 deletions platforms/metagram/src/lib/ui/Input/Input.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
interface IInputProps extends HTMLInputAttributes {
type: HTMLInputTypeAttribute;
selected?: string;
name?: string;
input?: HTMLInputElement;
value: string | number | any;
placeholder?: string;
Expand All @@ -13,21 +15,63 @@
type = 'text',
input = $bindable(),
value = $bindable(),
selected = $bindable(),
name = '',
placeholder = '',
...restProps
}: IInputProps = $props();
const cbase = $derived(
'w-full bg-grey py-3.5 px-6 text-[15px] text-black-800 font-geist font-normal placeholder:text-black-600 rounded-4xl outline-0 border border-transparent invalid:border-red invalid:text-red focus:invalid:text-black-800 focus:invalid:border-transparent'
);
let radioElement: HTMLInputElement | null = $state(null);
const typeClasses: Record<string, string> = {
radio: 'opacity-100'
};
const cbase = $derived({
common: 'w-full bg-grey py-3.5 px-6 text-[15px] text-black-800 font-geist font-normal placeholder:text-black-600 rounded-4xl outline-0 border border-transparent invalid:border-red invalid:text-red focus:invalid:text-black-800 focus:invalid:border-transparent',
type: typeClasses[type]
});
const radioCustomStyles = $derived({
common: "before:h-4.5 before:w-4.5 before:border-brand-burnt-orange before:-left-0.75 before:-bottom-0.25 relative before:absolute before:rounded-full before:border-2 before:bg-white before:content-['']",
selected:
'after:h-2.5 after:w-2.5 after:bg-brand-burnt-orange after:absolute after:bottom-0.75 after:left-0.25 after:rounded-full'
});
</script>

<input
{...restProps}
{type}
{placeholder}
bind:value
bind:this={input}
class={cn([cbase, restProps.class].join(' '))}
tabindex="0"
/>
{#if type === 'radio'}
<div
class={cn(
[radioCustomStyles.common, selected === value ? radioCustomStyles.selected : ''].join(
' '
)
)}
aria-checked={selected === value}
role="radio"
tabindex="0"
onclick={() => radioElement?.click()}
onkeypress={() => radioElement?.click()}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Improve keyboard accessibility and event handling.

The current keyboard handling has several issues:

  1. onkeypress doesn't handle all keyboard interactions (missing Enter and Space)
  2. Event delegation could cause unexpected behavior
  3. Missing proper focus management
-aria-checked={selected === value}
-role="radio"
-tabindex="0"
-onclick={() => radioElement?.click()}
-onkeypress={() => radioElement?.click()}
+aria-checked={selected === value}
+role="radio"
+tabindex="0"
+onclick={() => hiddenRadioRef?.click()}
+onkeydown={(e) => {
+    if (e.key === 'Enter' || e.key === ' ') {
+        e.preventDefault();
+        hiddenRadioRef?.click();
+    }
+}}

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents 
In platforms/metagram/src/lib/ui/Input/Input.svelte around lines 45 to 49,
improve keyboard accessibility by replacing the onkeypress handler with an
onkeydown handler that explicitly checks for Enter and Space keys to trigger the
click event. Avoid direct event delegation by ensuring the handler is bound to
the correct element. Additionally, manage focus properly by setting tabindex and
ensuring the element is focusable and visually indicates focus state.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you wont have to do this, also we are extending only input, do not add a shadow div parent, this is a component not a fragment

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The shadow div is for styling and creating the custom radio input.

>
<input
{...restProps}
type="radio"
{value}
bind:group={selected}
bind:this={radioElement}
{name}
checked={selected === value}
class={cn([cbase.common, cbase.type, restProps.class].join(' '))}
tabindex="0"
/>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Fix radio input accessibility and remove redundant properties.

The radio input implementation has several issues:

  1. Two elements with tabindex="0" creates confusing tab order
  2. Redundant checked attribute when using bind:group
  3. The hidden input should not be focusable
 <input
     {...restProps}
     type="radio"
     {value}
     bind:group={selected}
-    bind:this={radioElement}
+    bind:this={hiddenRadioRef}
     {name}
-    checked={selected === value}
     class={cn([cbase.common, cbase.type, restProps.class].join(' '))}
-    tabindex="0"
+    tabindex="-1"
+    style="position: absolute; opacity: 0; pointer-events: none;"
 />
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
<input
{...restProps}
type="radio"
{value}
bind:group={selected}
bind:this={radioElement}
{name}
checked={selected === value}
class={cn([cbase.common, cbase.type, restProps.class].join(' '))}
tabindex="0"
/>
<input
{...restProps}
type="radio"
{value}
bind:group={selected}
bind:this={hiddenRadioRef}
{name}
class={cn([cbase.common, cbase.type, restProps.class].join(' '))}
tabindex="-1"
style="position: absolute; opacity: 0; pointer-events: none;"
/>
🤖 Prompt for AI Agents 
In platforms/metagram/src/lib/ui/Input/Input.svelte around lines 51 to 61, fix
the radio input accessibility by removing the redundant checked attribute since
bind:group already manages selection, remove tabindex="0" from the radio input
to avoid duplicate focusable elements, and ensure the hidden input element is
not focusable by removing or setting tabindex to -1. This will correct tab order
confusion and improve accessibility.

</div>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider adding ARIA labelledby or label association.

The custom radio implementation lacks proper label association, which is crucial for screen readers. Consider adding support for labels or ARIA attributes.

Add label support to the interface and implementation:

 interface IInputProps extends HTMLInputAttributes {
     type: HTMLInputTypeAttribute;
     selected?: string;
     name?: string;
+    label?: string;
+    id?: string;
 }

Then in the radio rendering:

 <div
     class={cn(
         [radioCustomStyles.common, selected === value ? radioCustomStyles.selected : ''].join(' ')
     )}
     aria-checked={selected === value}
     role="radio"
+    aria-labelledby={id ? `${id}-label` : undefined}
     tabindex="0"

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents 
In platforms/metagram/src/lib/ui/Input/Input.svelte between lines 38 and 62, the
custom radio input lacks proper label association for accessibility. To fix
this, add a label prop to the component interface and associate the radio input
with a label element or use ARIA attributes like aria-labelledby or aria-label.
Update the radio rendering to wrap the input in a label or link it via
aria-labelledby to ensure screen readers can correctly identify the radio
button's label.

{:else}
<input
{...restProps}
{type}
{placeholder}
bind:this={input}
bind:value
class={cn([cbase.common, cbase.type, restProps.class].join(' '))}
tabindex="0"
/>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the way i mentioned it, this stays like before, rest goes

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bind:group doesn't work with dynamic 2 way binding and hence can't use the same input for the radio button too as the type attribute in this is a dynamic 2 way binding.
image

{/if}