Skip to content

fix: download button icon#6395

Merged
AugustinMauroy merged 2 commits intonodejs:mainfrom
heygsc:main
Mar 2, 2024
Merged

fix: download button icon#6395
AugustinMauroy merged 2 commits intonodejs:mainfrom
heygsc:main

Conversation

@heygsc
Copy link
Contributor

@heygsc heygsc commented Mar 2, 2024 •
edited
Loading

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.

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

vercel bot commented Mar 2, 2024 •
edited
Loading

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
Member

AugustinMauroy commented Mar 2, 2024 •
edited
Loading

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

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

@heygsc
fix: download button icon
18552ab 
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.

@AugustinMauroy AugustinMauroy added the github_actions:pull-request Trigger Pull Request Checks label Mar 2, 2024
@github-actions github-actions bot removed the github_actions:pull-request Trigger Pull Request Checks label Mar 2, 2024
@github-actions
Copy link
Contributor

github-actions bot commented Mar 2, 2024 •
edited
Loading

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 🔗

AugustinMauroy
AugustinMauroy approved these changes Mar 2, 2024
View reviewed changes
Copy link
Member

@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 🎉

@github-actions
Copy link
Contributor

github-actions bot commented Mar 2, 2024 •
edited
Loading

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!

@AugustinMauroy AugustinMauroy mentioned this pull request Mar 2, 2024
Closed
@uncenter
Copy link

uncenter commented Mar 2, 2024 •
edited
Loading

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 •
edited
Loading

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 •
edited
Loading

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

@ovflowd ovflowd added the github_actions:pull-request Trigger Pull Request Checks label Mar 2, 2024
@github-actions github-actions bot removed the github_actions:pull-request Trigger Pull Request Checks label Mar 2, 2024
shanpriyan
shanpriyan approved these changes Mar 2, 2024
View reviewed changes
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@AugustinMauroy AugustinMauroy AugustinMauroy approved these changes

+1 more reviewer

@shanpriyan shanpriyan shanpriyan approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

fast-track Fast Tracking PRs

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Confusing "Download Node.js" button on homepage

5 participants

@heygsc @AugustinMauroy @uncenter @ovflowd @shanpriyan