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

feat: default transition (for colors) to 0.3s #6726

Merged
merged 4 commits into from
May 25, 2024
Merged

Conversation

RedYetiDev
Copy link
Member

@RedYetiDev RedYetiDev commented May 18, 2024

Description

This PR sets the default transition to 0.3s

Validation

To validate whether a transition exists, hover over some elements, and verify a 0.3s fade from their properties.

Check List

  • I have read the Contributing Guidelines and made commit messages that follow the guideline.
  • I have run npm run format to ensure the code follows the style guide.
  • I have run npm run test to check if all tests are passing.

    Note: SyntaxError: Unexpected identifier 'assert', might be a local Node.js issue

  • I have run npx turbo build to check if the website builds without errors.

    Note: SyntaxError: Unexpected identifier 'assert', might be a local Node.js issue

  • I've covered new added functionality with unit tests if necessary.

@RedYetiDev RedYetiDev requested a review from a team as a code owner May 18, 2024 13:49
Copy link

vercel bot commented May 18, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
nodejs-org ✅ Ready (Inspect) Visit Preview May 21, 2024 8:09pm

@RedYetiDev
Copy link
Member Author

The only visual issue I am seeing is with the language selector.

@AugustinMauroy
Copy link
Contributor

Note: SyntaxError: Unexpected identifier 'assert', might be a local Node.js issue

Nodejs website use V20 so the error should come from you node version

@RedYetiDev
Copy link
Member Author

RedYetiDev commented May 18, 2024

Note: SyntaxError: Unexpected identifier 'assert', might be a local Node.js issue

Nodejs website use V20 so the error should come from you node version

Good to note, thanks

Signed-off-by: Aviv Keller <38299977+RedYetiDev@users.noreply.github.com>
Signed-off-by: Aviv Keller <38299977+RedYetiDev@users.noreply.github.com>
@ovflowd
Copy link
Member

ovflowd commented May 18, 2024

Why is this a fix? What is it fixing? Could you please attach screenshots?

@RedYetiDev
Copy link
Member Author

Why is this a fix?

It's a feature (feat)

What is it fixing?

N/A

Could you please attach screenshots?

Because it's a transitional effect, screenshots aren't possible, would you like a little video recording?

@ovflowd
Copy link
Member

ovflowd commented May 19, 2024

Why is this a fix?

It's a feature (feat)

What is it fixing?

N/A

Could you please attach screenshots?

Because it's a transitional effect, screenshots aren't possible, would you like a little video recording?

Sure; Please add more details explaining why is this needed, what is this improving and before/after videos of what is affecting.

@AugustinMauroy
Copy link
Contributor

If I've understood correctly @RedYetiDev you're trying to avoid the flash between the ligh/dark theme?

@RedYetiDev
Copy link
Member Author

If I've understood correctly @RedYetiDev you're trying to avoid the flash between the ligh/dark theme?

Sorry for the delay in my response, yes that is part of the reason, but also other things (like hover effects). I’ll make a video difference soon

@RedYetiDev
Copy link
Member Author

New

screen-capture.webm

Old

screen-capture.1.webm

@ovflowd
Copy link
Member

ovflowd commented May 21, 2024

I see, I like the changes. But since this changes every page; Please ensure to test every page for inconsistencies or visual bugs this change might introduce.

Copy link

github-actions bot commented May 21, 2024

Lighthouse Results

URL Performance Accessibility Best Practices SEO Report
/en 🟢 98 🟢 100 🟢 100 🟢 91 🔗
/en/about 🟢 100 🟢 100 🟢 100 🟢 91 🔗
/en/about/previous-releases 🟢 97 🟢 100 🟢 100 🟢 92 🔗
/en/download 🟢 94 🟢 100 🟢 100 🟢 91 🔗
/en/blog 🟢 99 🟢 100 🟢 100 🟢 92 🔗

Copy link
Member

@ovflowd ovflowd left a comment

Choose a reason for hiding this comment

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

LGTM; I understand the purpose of the changes now. But I'd recommend the author to discretionarily test the website (and all its pages, buttons, etc) to ensure that this change does not introduce issues.

Copy link

github-actions bot commented May 21, 2024

Unit Test Coverage Report

Lines Statements Branches Functions
Coverage: 91%
90.06% (589/654) 76.08% (175/230) 92.24% (119/129)

Unit Test Report

Tests Skipped Failures Errors Time
128 0 💤 0 ❌ 0 🔥 6.695s ⏱️

@RedYetiDev
Copy link
Member Author

LGTM; I understand the purpose of the changes now. But I'd recommend the author to discretionarily test the website (and all its pages, buttons, etc) to ensure that this change does not introduce issues.

Currently, it introduces one issue (with a weird transition for the language selector), but I'll look for more, than fix what needs to be done

Copy link
Contributor

@AugustinMauroy AugustinMauroy left a comment

Choose a reason for hiding this comment

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

Pleas use tailwindcss !transition-none and duration-300

@AugustinMauroy
Copy link
Contributor

Enregistrement.de.l.ecran.2024-05-21.a.14.18.11.mov

Also there this strange behavior

@RedYetiDev
Copy link
Member Author

The dropdowns are experiencing strange behavior, I need to fix that.

Once I do that I'll also move to tailwind

@RedYetiDev RedYetiDev changed the title feat: default transition to 0.3s feat: default transition (for colors) to 0.3s May 21, 2024
@ovflowd
Copy link
Member

ovflowd commented May 22, 2024

The dropdowns are experiencing strange behavior, I need to fix that.

Once I do that I'll also move to tailwind

I appreciate your thoroughness!

Copy link
Contributor

@AugustinMauroy AugustinMauroy left a comment

Choose a reason for hiding this comment

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

LGTM !

@ovflowd ovflowd added this pull request to the merge queue May 25, 2024
@@ -1,5 +1,6 @@
* {
@apply subpixel-antialiased;
@apply subpixel-antialiased
transition-colors;
Copy link
Member

Choose a reason for hiding this comment

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

Looks good, nice touch! @RedYetiDev I have one concern for users sensitive to this type of interaction, it may be better to use motion-safe:transition-colors. This feature allows us to control it via the OS settings

https://developer.mozilla.org/en-US/docs/Web/CSS/@media/prefers-reduced-motion

https://tailwindcss.com/docs/hover-focus-and-other-states#prefers-reduced-motion

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice note, seeing as this already landed, a second PR can always change it if needed

Merged via the queue into nodejs:main with commit 5d23642 May 25, 2024
15 checks passed
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.

None yet

4 participants