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: download button icon #6395

Merged
merged 2 commits into from Mar 2, 2024
Merged

fix: download button icon #6395

merged 2 commits into from Mar 2, 2024

Conversation

heygsc
Copy link
Contributor

@heygsc heygsc commented Mar 2, 2024

Description

Before the download button, it was pointed to the right, but changed to downward, which is in line with the meaning of downloading.

Validation

Related Issues

fix: #6392

Check List

  • I have read the Contributing Guidelines and made commit messages that follow the guideline.
  • I have run npx turbo lint to ensure the code follows the style guide. And run npx turbo lint:fix to fix the style errors if necessary.
  • I have run npx turbo format to ensure the code follows the style guide.
  • I have run npx turbo test to check if all tests are passing.
  • I've covered new added functionality with unit tests if necessary.

Signed-off-by: heygsc <1596920983@qq.com>
@heygsc heygsc requested a review from a team as a code owner March 2, 2024 16:54
Copy link

vercel bot commented Mar 2, 2024

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

Name Status Preview Updated (UTC)
nodejs-org ✅ Ready (Inspect) Visit Preview Mar 2, 2024 5:04pm

@AugustinMauroy
Copy link
Contributor

AugustinMauroy commented Mar 2, 2024

I'm not fan of this icon we should use same icon as download page.

import { CloudArrowDownIcon } from '@heroicons/react/24/outline';

Signed-off-by: heygsc <1596920983@qq.com>
@heygsc
Copy link
Contributor Author

heygsc commented Mar 2, 2024

I'm not fan of this icon we should use same icon as download page.

import { CloudArrowDownIcon } from '@heroicons/react/24/outline';

It makes sense.
At first, when I saw several download buttons on the tailwind, I didn't know which one to choose.
The download page was indeed suitable. I updated it again.

Copy link

github-actions bot commented Mar 2, 2024

Lighthouse Results

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

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.

SGTM ! Thanks for your first contribution 🎉

Copy link

github-actions bot commented Mar 2, 2024

Unit Test Coverage Report

Lines Statements Branches Functions
Coverage: 85%
80.18% (433/540) 79.65% (137/172) 73.07% (76/104)

Unit Test Report

Tests Skipped Failures Errors Time
88 0 💤 0 ❌ 0 🔥 4.502s ⏱️

@heygsc heygsc changed the title fix: download button direction fix: download button icon Mar 2, 2024
@heygsc
Copy link
Contributor Author

heygsc commented Mar 2, 2024

SGTM ! Thanks for your first contribution 🎉

Thank you!

@uncenter
Copy link

uncenter commented Mar 2, 2024

FYI if you add "closes #6392" to the PR description it will show up as a relevant PR in the issue itself and automatically close the issue when the PR is merged. (Prevents confusion like this: #6392 (comment).)

@ovflowd
Copy link
Member

ovflowd commented Mar 2, 2024

FYI, can we swap the icon to before the text? For the arrow, it made sense to be on the right, not for this icon anymore.

@uncenter
Copy link

uncenter commented Mar 2, 2024

I kinda think on the right makes sense?

@ovflowd
Copy link
Member

ovflowd commented Mar 2, 2024

I kinda think on the right makes sense?

Why so? (Out of curiosity)

@heygsc
Copy link
Contributor Author

heygsc commented Mar 2, 2024

FYI if you add "closes #6392" to the PR description it will show up as a relevant PR in the issue itself and automatically close the issue when the PR is merged. (Prevents confusion like this: #6392 (comment).)

Okay, I didn't carefully check the issue before, but now it has been added to the description.

@heygsc
Copy link
Contributor Author

heygsc commented Mar 2, 2024

FYI, can we swap the icon to before the text? For the arrow, it made sense to be on the right, not for this icon anymore.

To the right, is it to point to the "download" text?
Haha, maybe it can be discussed again.

@uncenter
Copy link

uncenter commented Mar 2, 2024

I kinda think on the right makes sense?

Why so? (Out of curiosity)

It just feels right 😅

Right Left
Capture-2024-03-02-131119 image

@ovflowd
Copy link
Member

ovflowd commented Mar 2, 2024

I kinda think on the right makes sense?

Why so? (Out of curiosity)

It just feels right 😅

Right Left
Capture-2024-03-02-131119 image

Indeed it looks right. (Ba dum ts)

@ovflowd ovflowd added the fast-track Fast Tracking PRs label Mar 2, 2024
@ovflowd
Copy link
Member

ovflowd commented Mar 2, 2024

I'm fast-tracking this since it's a minor fix. Thank you for your first contribution, @heygsc and thank you for reporting this issue, @uncenter

Copy link
Contributor

@shanpriyan shanpriyan left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for the PR 🌟

@AugustinMauroy AugustinMauroy added this pull request to the merge queue Mar 2, 2024
Merged via the queue into nodejs:main with commit 2f6e08b Mar 2, 2024
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fast-track Fast Tracking PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Confusing "Download Node.js" button on homepage
5 participants