-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
[Admin] Add toggle switch component #5299
Conversation
Codecov Report
@@ Coverage Diff @@
## nebulab/admin #5299 +/- ##
==============================================
Coverage 88.56% 88.57%
==============================================
Files 599 600 +1
Lines 14573 14581 +8
==============================================
+ Hits 12907 12915 +8
Misses 1666 1666
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
3e2c888
to
8f16920
Compare
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.
Great work! It's awesome to see components come alive ✨
I left a couple of comments over internal details, but looks good overall 🙌
admin/app/components/solidus_admin/ui/forms/switch/component.rb
Outdated
Show resolved
Hide resolved
<input type="checkbox" class="h-0 w-0 invisible peer" id="<%= @id %>" <%= @states.join(' ') %>/> | ||
<label for="<%= @id %>" class="<%= @label_class %>"></label> |
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.
I saw this is the most common implementation of a toggle switch, would be great if there was an alternative only using the input
element (e.g. by using SVG backgrounds, like the checkbox does) or one in which there was a single root (e.g. the label <label><input …/></label>
).
Having a single root makes it simpler to organize the component from outside (e.g. putting it in a flex container and expect it to be a single element).
At worst maybe we can wrap it into a div
or span
.
Thoughts?
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.
the problem on putting the <input>
inside the <label>
tag is that the Tailwind peer
won't work, since it requires the peer to be defined first (which is the <input>
tag), so the label can change depending on its status.
the tag on the current solution is 0x0 with an invisible
property, which shouldn't affect much, but it's definitely better to keep as one tag.
I'll study your first suggestion, in case I'm not successful I'll stick with wrapping it on a <div>
tag.
--
Update:
Managed to reduce to only an input
element :)
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.
That's awesome!!!!
8f16920
to
8d035a5
Compare
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.
🚀 🚀 🚀
admin/app/components/solidus_admin/ui/forms/switch/component.rb
Outdated
Show resolved
Hide resolved
<input type="checkbox" class="h-0 w-0 invisible peer" id="<%= @id %>" <%= @states.join(' ') %>/> | ||
<label for="<%= @id %>" class="<%= @label_class %>"></label> |
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.
That's awesome!!!!
8d035a5
to
79e8821
Compare
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.
Great job! 👏
Summary
recording.mov
Checklist
Check out our PR guidelines for more details.
The following are mandatory for all PRs:
The following are not always needed: