💥 395: add new required variable id to all form fields to ensure a11y #396
Conversation
lehju
added
the
Accessibility
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe pull request focuses on enhancing the accessibility of form components across multiple Vue files. By introducing a new Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
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
Documentation and Community
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![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
🧹 Nitpick comments (10)
src/components/Form/MucTextArea.stories.ts (1)
26-31: Consider renaming the 'Error' story to avoid shadowingThank you for implementing the error state story! To make the code even better, would you consider renaming this story to something like
WithErrororErrorState? This would prevent shadowing the globalErrorproperty and make the code more maintainable.Here's a suggested change:
-export const Error = { +export const WithError = { args: { ...Default.args, id: "error", errorMsg: "An error occured", }, };🧰 Tools
🪛 Biome (1.9.4)
[error] 26-26: Do not shadow the global "Error" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
src/components/Form/MucTextArea.vue (3)
23-25: Consider conditionally applying aria-describedby 🤔Thank you for adding the accessibility attributes! One small suggestion to make it even better: consider conditionally applying the
aria-describedbyattribute only when a hint exists.Here's how you could improve it:
<textarea :id="'textarea-' + id" class="m-textarea" - :aria-describedby="'textarea-hint-' + id" + :aria-describedby="hint ? 'textarea-hint-' + id : undefined" :rows="rows" :placeholder="placeholder" v-model="modelValue" />
31-34: Great addition of keyboard accessibility! Here's a suggestion to make it even better 🌟Thank you for making the error message focusable! To make it even more accessible, consider adding
role="alert"andaria-live="polite"to ensure screen readers announce the error when it appears.Here's the suggested enhancement:
<form-error-message v-if="errorMsg" tabindex="0" + role="alert" + aria-live="polite" > {{ errorMsg }} </form-error-message>
53-56: Thank you for documenting the new prop! Here's a suggestion to make it even clearer 📝The JSDoc comment for the
idprop could be more descriptive to help other developers understand its purpose and importance for accessibility.Here's a suggested enhancement:
/** - * id of textarea + * Unique identifier for the textarea. Used to associate the textarea with its label and hint text for accessibility. */ id: string;src/components/Form/MucCheckbox.stories.ts (1)
23-23: Thank you for adding the ID! Consider making it more specific.While adding an ID is great for accessibility, using "default" might be too generic if multiple checkboxes are rendered on the same page. Consider using a more descriptive ID that reflects the checkbox's purpose, like "terms-acceptance" or "newsletter-signup".
- id: "default", + id: "checkbox-story-default",src/components/Form/MucCheckbox.vue (1)
7-7: Wonderful implementation of accessible checkboxes! 🌟Great job on implementing the ID prop and correctly associating the label with the input element. The documentation is clear, and the 'checkbox-' prefix helps prevent ID collisions.
One small suggestion to make this even better:
Consider adding prop validation to ensure the ID is always provided and follows a valid format:
const { label } = defineProps<{ /** * id of checkbox */ - id: string; + id: { + type: String, + required: true, + validator: (value: string) => /^[a-zA-Z][\w-]*$/.test(value) + }; /** * Label is displayed to the right of the checkbox as information for the user. */ label: string; }>();Also applies to: 14-17, 29-33
src/components/Form/MucCheckboxGroup.stories.ts (1)
25-25: Consider using more descriptive IDs for better accessibilityThank you for adding IDs to improve accessibility! However, using just the index number as an ID might not be descriptive enough for screen readers. Consider using a more descriptive format similar to the Collapsable story.
- <MucCheckbox v-for="index in 4" :key="index" :label="'not-collapsed-' + index" :id="index"/> + <MucCheckbox v-for="index in 4" :key="index" :label="'not-collapsed-' + index" :id="'not-collapsed-' + index"/>src/components/Form/MucInput.stories.ts (1)
88-92: Consider renaming the Date story to avoid shadowingTo prevent potential confusion with the global
Dateobject, consider using a more specific name.-export const Date = { +export const DateInput = { args: { id: "date", type: "date", }, };🧰 Tools
🪛 Biome (1.9.4)
[error] 88-88: Do not shadow the global "Date" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
src/components/Form/MucSelect.vue (2)
Line range hint
32-44: Consider enhancing trigger button accessibilityThe trigger button could benefit from additional accessibility attributes:
<span class="m-input__trigger" @click="toggleItemList" + role="button" + :aria-expanded="showItems" + :aria-label="'Toggle ' + (label || 'select') + ' dropdown'" >This would:
- Properly identify it as a button for screen readers
- Indicate its expanded/collapsed state
- Provide clear context about its purpose
121-124: Consider enhancing the id prop documentation and validationThank you for adding the
idprop! To make it even more robust, consider:/** * id of select + * @remarks Must be unique across the page to ensure proper accessibility */ -id: string; +id: { + type: String, + required: true, + validator: (value: string) => value.trim().length > 0 +}This would:
- Enforce non-empty strings
- Clarify the uniqueness requirement
- Maintain type safety
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/components/Form/MucCheckbox.stories.ts(1 hunks)src/components/Form/MucCheckbox.vue(2 hunks)src/components/Form/MucCheckboxGroup.stories.ts(2 hunks)src/components/Form/MucInput.stories.ts(4 hunks)src/components/Form/MucInput.vue(4 hunks)src/components/Form/MucRadioButton.stories.ts(1 hunks)src/components/Form/MucRadioButton.vue(2 hunks)src/components/Form/MucSelect.stories.ts(2 hunks)src/components/Form/MucSelect.vue(3 hunks)src/components/Form/MucTextArea.stories.ts(2 hunks)src/components/Form/MucTextArea.vue(2 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/components/Form/MucTextArea.stories.ts
[error] 26-26: Do not shadow the global "Error" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
src/components/Form/MucInput.stories.ts
[error] 88-88: Do not shadow the global "Date" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
🔇 Additional comments (15)
src/components/Form/MucTextArea.stories.ts (1)
21-21: Thank you for adding unique IDs to improve accessibility! 🎉The addition of descriptive IDs for each story variant is a great improvement for accessibility. These IDs will help ensure proper association between labels, inputs, and hints for screen readers.
Also applies to: 29-29, 38-38, 46-46
src/components/Form/MucTextArea.vue (2)
8-8: Thank you for improving label accessibility! 🎉The dynamic binding of the label's
forattribute to the textarea ID is a great accessibility enhancement. This ensures proper association between the label and textarea, making it easier for screen readers to announce the label when the textarea receives focus.
14-18: Excellent work on the hint implementation! 💫Great improvements here:
- The
v-if="hint"prevents empty hint elements from being rendered- The unique ID enables proper association with the textarea via aria-describedby
This makes the component more semantic and screen-reader friendly.
src/components/Form/MucRadioButton.stories.ts (1)
27-28: Excellent work on the radio button IDs! 👏Great implementation of unique IDs for both the static and dynamically generated radio buttons. This will significantly improve the screen reader experience.
src/components/Form/MucSelect.stories.ts (1)
21-21: Great work on providing unique IDs for each select variant! ✨I appreciate how you've given each select component a unique, purpose-describing ID. This is exactly what we need for proper accessibility support.
Also applies to: 58-58, 66-66
src/components/Form/MucCheckboxGroup.stories.ts (1)
36-40: Great job on descriptive IDs! 👍The IDs in the Collapsable story are well-structured and descriptive, making it clear which checkboxes belong to which section. This is excellent for accessibility!
src/components/Form/MucRadioButton.vue (2)
7-24: Excellent accessibility implementation! 🎉Really appreciate the thorough implementation of accessibility features:
- Proper label association using
forattribute- ARIA description linking with
aria-describedby- Consistent ID prefixing pattern with 'radio-'
37-40: Well-documented prop definitionThank you for adding clear documentation for the new
idprop. This helps other developers understand its purpose and importance.src/components/Form/MucInput.stories.ts (1)
Line range hint
23-99: Great work on unique, descriptive IDs! 🌟Really appreciate how each story variant has a meaningful, context-specific ID. This makes the stories more accessible and easier to test.
🧰 Tools
🪛 Biome (1.9.4)
[error] 88-88: Do not shadow the global "Date" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
src/components/Form/MucInput.vue (3)
Line range hint
8-38: Comprehensive accessibility implementation! 🎯Excellent work on implementing all the necessary accessibility features:
- Proper label association with
forattribute- Unique IDs for input and hint elements
- ARIA description linking with
aria-describedby
69-69: Good addition of keyboard accessibilityThank you for making the error message keyboard-focusable with
tabindex="0". This ensures that keyboard users can access error messages easily.
102-105: Clear prop documentationAppreciate the clear documentation for the new
idprop. This helps maintain code quality and helps other developers understand its purpose.src/components/Form/MucSelect.vue (3)
6-10: Great job on implementing proper label-input association! 🎉The label is correctly associated with the input using the
forattribute, which is essential for accessibility. The dynamic ID generation using'select-' + idensures uniqueness.
14-16: Well done on implementing hint text accessibility! 👏The hint text is properly associated with the input using a unique ID, which will help screen reader users understand the input's purpose.
25-25: Perfect implementation of input accessibility attributes! ✨The input element correctly uses both:
- Unique ID matching the label's
forattributearia-describedbylinking to the hint textAlso applies to: 28-28
|
rest is looking good! |
|
🎉 This PR is included in version 3.0.0-beta.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Description
Add ids to form fields so that the screen reader can access the labels and hints.
Reference
closes #395
Summary by CodeRabbit
New Features
idattributes to form components (Checkbox, Input, Radio Button, Select, TextArea) to improve accessibility and component identification.Accessibility Improvements
idbindingsaria-describedbyattributes for better screen reader support