Skip to content
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

Merged
merged 1 commit into from
Sep 28, 2024

Conversation

bluzky
Copy link
Owner

@bluzky bluzky commented Sep 28, 2024

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:

  • Replace native radio input with a custom wrapper element to ensure consistent UI across different browsers.

Copy link

sourcery-ai bot commented Sep 28, 2024

Reviewer's Guide by Sourcery

This 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 radio_group_item function within the SaladUI.RadioGroup module.

Sequence Diagram

sequenceDiagram
    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
Loading

File-Level Changes

Change Details Files
Replace default radio input with custom styled label and hidden input
  • Wrap radio input in a label element with custom styling
  • Hide the original radio input and use it as a peer element
  • Add a span element with an SVG icon to represent the radio button state
  • Implement custom styling for focus and disabled states
lib/salad_ui/radio_group.ex
Update CSS classes for improved styling and interactivity
  • Add focus-related classes for better keyboard navigation
  • Implement peer classes for styling based on radio button state
  • Add classes for handling disabled state
lib/salad_ui/radio_group.ex

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it.

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@bluzky bluzky merged commit 0cf3c06 into main Sep 28, 2024
2 checks passed
@bluzky bluzky deleted the feature/improve-radio-group branch September 28, 2024 16:00
Copy link

@sourcery-ai sourcery-ai bot left a 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

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
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
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant