-
-
Notifications
You must be signed in to change notification settings - Fork 47
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.
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
Improve radio input, consistent UI cross browser #44
Conversation
Reviewer's Guide by SourceryThis pull request improves the radio input component by replacing the default HTML radio input with a custom wrapper element. The main goal is to achieve a consistent UI across different browsers. The changes are implemented in the Sequence DiagramsequenceDiagram
participant User
participant Label
participant HiddenInput
participant SVGIcon
User->>Label: Clicks on radio button
Label->>HiddenInput: Triggers hidden input
HiddenInput->>SVGIcon: Updates checked state
SVGIcon->>User: Displays filled circle
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @bluzky - I've reviewed your changes - here's some feedback:
Overall Comments:
- Please ensure that this custom radio input implementation remains accessible to screen readers and other assistive technologies. Consider testing with various screen readers to confirm compatibility.
- The inline SVG for the radio button indicator adds complexity to the template. Consider extracting this into a separate component or helper function to improve readability and maintainability.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
@@ -53,18 +53,36 @@ defmodule SaladUI.RadioGroup do | |||
|
|||
def radio_group_item(assigns) do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: Consider renaming radio_group_item for consistency
For better consistency with the module name (SaladUI.RadioGroup), consider renaming radio_group_item to just item. This would make the context clearer and improve overall naming consistency in the module.
def item(assigns) do
I replace radio input with custom wapper element, so the UI should be consistent cross browser
Summary by Sourcery
Enhance the radio input component by replacing the native input with a custom wrapper element, improving cross-browser UI consistency.
Enhancements: