Enhance comment forms #604
Conversation
WalkthroughThis pull request focuses on enhancing the accessibility and correctness of comment form components across different form types (Horizontal, Inline, and Stacked) in the comments module. The changes involve adding ARIA attributes to form elements, correcting label-input associations, and fixing a component rendering typo in the main CommentForm component. These modifications improve screen reader compatibility and ensure proper form interaction without altering the core functionality of the forms. Changes
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
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 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: 2
🧹 Outside diff range and nitpick comments (4)
client/app/bundles/comments/rescript/CommentForm/forms/InlineForm.res (1)
37-42: Consider using a button element instead of input type="submit"While the current implementation works, using a
buttonelement withtype="submit"is considered better practice for form submissions. It offers better styling flexibility and semantic meaning.Here's the suggested change:
- <input - type_="submit" + <button + type_="submit" className="self-start px-3 py-1 font-semibold border-0 rounded text-sky-50 bg-sky-600 hover:bg-sky-800" - value="Post" aria-label="Submit comment" - /> + > + {"Post"->React.string} + </button>client/app/bundles/comments/rescript/CommentForm/forms/StackedFrom.res (1)
19-19: Consider a more descriptive label for the comment fieldWhile the accessibility improvements are good, consider changing the label from "Text" to "Comment" or "Comment Text" to be more descriptive of the field's purpose.
- <label htmlFor="comment_text" className="w-full"> {"Text"->React.string} </label> + <label htmlFor="comment_text" className="w-full"> {"Comment"->React.string} </label>Also applies to: 28-29
client/app/bundles/comments/rescript/CommentForm/forms/HorizontalForm.res (2)
5-5: LGTM! Author field accessibility is well implemented.Good implementation of label-input association and aria attributes. Consider adding an asterisk (*) to visually indicate the required field, matching the aria-required state.
- <label htmlFor="comment_author" className="w-full lg:w-2/12 lg:text-end shrink-0"> {"Name"->React.string} </label> + <label htmlFor="comment_author" className="w-full lg:w-2/12 lg:text-end shrink-0"> {"Name *"->React.string} </label>Also applies to: 14-15
38-38: Consider using button element instead of input type="submit".While the aria-label addition is good, consider using a
buttonelement which is more semantic and provides better styling options.- <input - type_="submit" - className="self-start px-3 py-1 font-semibold border-0 rounded text-sky-50 bg-sky-600 hover:bg-sky-800" - value="Post" - aria-label="Submit comment" - /> + <button + type_="submit" + className="self-start px-3 py-1 font-semibold border-0 rounded text-sky-50 bg-sky-600 hover:bg-sky-800" + aria-label="Submit comment" + > + {"Post"->React.string} + </button>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
client/app/bundles/comments/rescript/CommentForm/forms/HorizontalForm.res(4 hunks)client/app/bundles/comments/rescript/CommentForm/forms/InlineForm.res(3 hunks)client/app/bundles/comments/rescript/CommentForm/forms/StackedFrom.res(3 hunks)
🔇 Additional comments (6)
client/app/bundles/comments/rescript/CommentForm/forms/InlineForm.res (2)
9-9: LGTM! Good accessibility implementation
The label and input are properly associated using htmlFor/id, and the aria attributes enhance screen reader support.
Also applies to: 18-19
23-23: LGTM! Consistent accessibility implementation
The comment text field follows the same good accessibility patterns as the author field.
Also applies to: 32-33
client/app/bundles/comments/rescript/CommentForm/forms/StackedFrom.res (3)
3-3: LGTM! Form accessibility enhancement
The addition of aria-label improves accessibility, and handling submission at the form level prevents potential double submissions.
5-5: LGTM! Author field accessibility improvements
The changes properly associate the label with the input field and add necessary ARIA attributes for screen readers.
Also applies to: 14-15
37-37: LGTM! Submit button accessibility enhancement
The addition of aria-label improves the button's accessibility, and handling submission at the form level prevents potential double submissions.
client/app/bundles/comments/rescript/CommentForm/forms/HorizontalForm.res (1)
3-3: LGTM! Form accessibility enhancement is well implemented.
The addition of aria-label on the form element improves screen reader context, and handling submission at the form level is the correct approach.
client/app/bundles/comments/rescript/CommentForm/forms/InlineForm.res
Outdated
Show resolved
Hide resolved
| /> | ||
| </div> | ||
| <div className="flex flex-col gap-0 items-center lg:gap-4 lg:flex-row"> | ||
| <label className="w-full lg:w-2/12 lg:text-end shrink-0"> {"Text"->React.string} </label> | ||
| <label htmlFor="comment_text" className="w-full lg:w-2/12 lg:text-end shrink-0"> {"Text"->React.string} </label> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using textarea for comment input.
While the accessibility improvements are great, a textarea would be more appropriate for comment text as it:
- Allows multiline input
- Provides better UX for markdown content
- Is semantically more correct for longer text input
- <input
- type_="text"
+ <textarea
className="px-3 py-1 leading-4 border border-gray-300 rounded w-full"
placeholder="Say something using markdown..."
name="comment_text"
id="comment_text"
onChange=handleTextChange
value={text}
aria-label="Comment text"
- aria-required="true"
+ aria-required="true"
+ rows=3
- />
+ />Also applies to: 28-29
The disabled prop is incorrectly passed without a value. In ReScript, boolean props need to be explicitly set.
- Use camelCase for aria attributes (ariaLabel, ariaRequired) - Use boolean `true` instead of string "true" for ariaRequired - Fix htmlFor placement inside label tags
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
client/app/bundles/comments/rescript/CommentForm/forms/StackedForm.res (3)
9-9: Consider enhancing the author input accessibility further.While the current accessibility improvements are good, consider adding these additional attributes for better screen reader support:
<input type_="text" className="px-3 py-1 leading-4 border border-gray-300 rounded w-full" placeholder="Your Name" name="comment_author" id="comment_author" onChange=handleAuthorChange value={author} ariaLabel="Author name" ariaRequired=true + role="textbox" + ariaInvalid={author->Js.String2.length == 0} />Also applies to: 18-19
23-23: Consider enhancing the comment text input accessibility further.Similar to the author input, consider adding additional accessibility attributes:
<input type_="text" className="px-3 py-1 leading-4 border border-gray-300 rounded w-full" placeholder="Say something using markdown..." name="comment_text" id="comment_text" onChange=handleTextChange value={text} ariaLabel="Comment text" ariaRequired=true + role="textbox" + ariaInvalid={text->Js.String2.length == 0} />Also applies to: 32-33
41-41: Consider enhancing the submit button accessibility.The submit button could benefit from additional accessibility attributes:
<input type_="submit" className="self-start px-3 py-1 font-semibold border-0 rounded text-sky-50 bg-sky-600 hover:bg-sky-800" value="Post" ariaLabel="Submit comment" + role="button" + ariaDisabled={disabled} />client/app/bundles/comments/rescript/CommentForm/CommentForm.res (1)
Line range hint
1-149: Consider enhancing form state management for better accessibility.While the current implementation handles basic form states well, consider adding form validation state to improve accessibility feedback:
- Add validation state to track empty required fields
- Pass validation state to form components to set
ariaInvalidappropriately- Add error messages for screen readers
Example enhancement:
type state = { author: string, text: string, form: formDisplay, savingStatus: savingStatus, + validationErrors: array<string>, } type action = | SetAuthor(string) | SetText(string) | SetFormType(formDisplay) | SetSavingError | ClearSavingError | SetStoreStatusSaving + | SetValidationErrors(array<string>) let reducer = (state: state, action: action): state => { switch action { | SetAuthor(author) => { ...state, author, + validationErrors: validateForm({...state, author}), } | SetText(text) => { ...state, text, + validationErrors: validateForm({...state, text}), } // ... other cases } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
client/app/bundles/comments/rescript/CommentForm/CommentForm.res(2 hunks)client/app/bundles/comments/rescript/CommentForm/forms/HorizontalForm.res(4 hunks)client/app/bundles/comments/rescript/CommentForm/forms/InlineForm.res(3 hunks)client/app/bundles/comments/rescript/CommentForm/forms/StackedForm.res(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- client/app/bundles/comments/rescript/CommentForm/forms/HorizontalForm.res
- client/app/bundles/comments/rescript/CommentForm/forms/InlineForm.res
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: test (22.x, 3.3.3)
- GitHub Check: test (22.x, 3.3.3)
- GitHub Check: test (22.x, 3.3.3)
🔇 Additional comments (2)
client/app/bundles/comments/rescript/CommentForm/forms/StackedForm.res (1)
3-7: LGTM! Form element accessibility improvements.Good addition of
ariaLabeland properdisabledattribute syntax. The form structure with flex layout provides a clean user interface.client/app/bundles/comments/rescript/CommentForm/CommentForm.res (1)
Line range hint
125-132: LGTM! Correct component name usage.The component name has been properly corrected from
StackedFromtoStackedForm, ensuring proper component rendering.
client/app/bundles/comments/rescript/CommentForm/forms/HorizontalForm.res
Outdated
Show resolved
Hide resolved
This PR improves the form components in the ReScript implementation by addressing several issues:
This change is
Summary by CodeRabbit
Accessibility Improvements
ariaLabelattributes to form elements, inputs, and submit buttonshtmlForattributesariaRequiredBug Fixes
StackedFromtoStackedFormdisabledattribute syntax for accuracy