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

fix: Switch Animation In Safari #553

Merged
merged 8 commits into from Jul 15, 2022
Merged

fix: Switch Animation In Safari #553

merged 8 commits into from Jul 15, 2022

Conversation

ghost
Copy link

@ghost ghost commented Jun 18, 2022

Closes #525

📝 Description

Add a brief description

⛳️ Current behavior (updates)

Please describe the current behavior that you are modifying

Switch circle is shaking when we off switch

🚀 New behavior

Please describe the behavior or changes this PR adds

💣 Is this a breaking change (Yes/No): No

📝 Additional Information

Shaking bug is solved but there is some lag because width animation is very heavy.
It's fine at Chrome but it seems Safari cannot process this kind of animation well.

Option1:
So I would like to suggest to remove width animation in active state.

Option2:
Or if you want to maintain current interaction.
We can change prop checked to css &:checked + .circle.

@ghost ghost requested a review from jrgarciadev as a code owner June 18, 2022 18:26
@ghost ghost marked this pull request as draft June 18, 2022 18:38
@ghost ghost marked this pull request as ready for review June 19, 2022 05:22
@ghost
Copy link
Author

ghost commented Jun 19, 2022

@jrgarciadev I apply option2 so we can maintain existing interactions with no lag or shaking bug

@jrgarciadev
Copy link
Member

Hey @TroyTae thank you so much, I'll check this out as soon as possible 🙏🏻

@codecov-commenter
Copy link

Codecov Report

Merging #553 (f71e7f4) into main (8eff522) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     #553   +/-   ##
=======================================
  Coverage   74.82%   74.82%           
=======================================
  Files         200      200           
  Lines        3098     3098           
  Branches      956      956           
=======================================
  Hits         2318     2318           
  Misses        765      765           
  Partials       15       15           
Impacted Files Coverage Δ
packages/react/src/switch/switch.tsx 93.93% <ø> (ø)
packages/react/src/switch/switch.styles.ts 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8eff522...f71e7f4. Read the comment docs.

@jrgarciadev jrgarciadev changed the base branch from main to next July 12, 2022 12:27
@jrgarciadev jrgarciadev added this to the 🚀 v1.0.0-beta.10 milestone Jul 12, 2022
@jrgarciadev
Copy link
Member

Hey @TroyTae 🙌🏼, could you please fix the conflicts and format the code with the new eslint rules?

@ghost
Copy link
Author

ghost commented Jul 12, 2022

@jrgarciadev I finished to fix conflicts and apply new eslint rules!
image
But two commits were included in this PR because I merged from main not next branch.
Is it okay? If not, I'll create new PR based on next branch.

@jrgarciadev
Copy link
Member

Hey @TroyTae thank you, I going to check it out, regarding which branch you should use before sending a PR, it depends on the change, if your changes don't include package/react changes, you can send the PR based on the main branch otherwise you should use the next branch because is the branch that we use for the next release

@jrgarciadev jrgarciadev changed the base branch from next to main July 15, 2022 14:37
@jrgarciadev jrgarciadev changed the base branch from main to next July 15, 2022 14:37
@jrgarciadev
Copy link
Member

Hey, @TroyTae sorry, I updated the next branch with the latest main changes, could you please update the PR?

@ghost
Copy link
Author

ghost commented Jul 15, 2022

@jrgarciadev Done! Thank you for updating :)

@jrgarciadev
Copy link
Member

Hey @TroyTae could you please remove the circle translation on the non-animated Switch?

Here's the issue:
https://user-images.githubusercontent.com/30373425/179251376-c1e0b8c3-9ada-426e-a596-4b214f4732f4.mov

@ghost
Copy link
Author

ghost commented Jul 15, 2022

@jrgarciadev Thank you I just finished to fix it 👍

Copy link
Member

@jrgarciadev jrgarciadev left a comment

Choose a reason for hiding this comment

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

Thank you so much @TroyTae 🙏🏻

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.

[BUG] - Switch Animation In Safari
2 participants