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

[Admin] Add toggle switch component #5299

Merged
merged 1 commit into from
Aug 4, 2023

Conversation

the-krg
Copy link
Contributor

@the-krg the-krg commented Aug 3, 2023

Summary

Screen Shot 2023-08-03 at 03 07 44
recording.mov
Screen Shot 2023-08-03 at 10 59 30

Checklist

Check out our PR guidelines for more details.

The following are mandatory for all PRs:

The following are not always needed:

  • 📖 I have updated the README to account for my changes.
  • 📑 I have documented new code with YARD.
  • 🛣️ I have opened a PR to update the guides.
  • ✅ I have added automated tests to cover my changes.
  • 📸 I have attached screenshots to demo visual changes.

@the-krg the-krg requested a review from elia August 3, 2023 06:28
@the-krg the-krg self-assigned this Aug 3, 2023
@codecov
Copy link

codecov bot commented Aug 3, 2023

Codecov Report

Merging #5299 (8d035a5) into nebulab/admin (226ea39) will increase coverage by 0.00%.
Report is 11 commits behind head on nebulab/admin.
The diff coverage is 100.00%.

❗ Current head 8d035a5 differs from pull request most recent head 79e8821. Consider uploading reports for the commit 79e8821 to get more accurate results

@@              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           
Files Changed Coverage Δ
...ponents/solidus_admin/ui/forms/switch/component.rb 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@the-krg the-krg force-pushed the the-krg/admin/admin-ui-switch branch from 3e2c888 to 8f16920 Compare August 3, 2023 13:58
@the-krg the-krg requested a review from rainerdema August 3, 2023 14:00
@the-krg the-krg marked this pull request as ready for review August 3, 2023 14:00
@the-krg the-krg requested a review from a team as a code owner August 3, 2023 14:00
Copy link
Member

@elia elia left a 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 🙌

Comment on lines 47 to 48
<input type="checkbox" class="h-0 w-0 invisible peer" id="<%= @id %>" <%= @states.join(' ') %>/>
<label for="<%= @id %>" class="<%= @label_class %>"></label>
Copy link
Member

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?

Copy link
Contributor Author

@the-krg the-krg Aug 3, 2023

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 :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's awesome!!!!

@the-krg the-krg force-pushed the the-krg/admin/admin-ui-switch branch from 8f16920 to 8d035a5 Compare August 4, 2023 06:02
@the-krg the-krg requested a review from elia August 4, 2023 06:04
Copy link
Member

@elia elia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀 🚀 🚀

Comment on lines 47 to 48
<input type="checkbox" class="h-0 w-0 invisible peer" id="<%= @id %>" <%= @states.join(' ') %>/>
<label for="<%= @id %>" class="<%= @label_class %>"></label>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's awesome!!!!

@elia elia force-pushed the the-krg/admin/admin-ui-switch branch from 8d035a5 to 79e8821 Compare August 4, 2023 09:33
Copy link
Contributor

@rainerdema rainerdema left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job! 👏

@rainerdema rainerdema merged commit cf6c2be into nebulab/admin Aug 4, 2023
2 checks passed
@rainerdema rainerdema deleted the the-krg/admin/admin-ui-switch branch August 4, 2023 10:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants