Enhance comment forms #604
Conversation
WalkthroughThe changes in this pull request involve modifications to the Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
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.
This PR improves the form components in the ReScript implementation by addressing several issues:
This change is
Summary by CodeRabbit
New Features
aria-labelattributes and linking labels to inputs.aria-required="true".Bug Fixes
These changes ensure a more inclusive experience for users utilizing assistive technologies.