FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

fix: download button icon by heygsc · Pull Request #6395 · nodejs/nodejs.org · GitHub

/ nodejs.org Public

fix: download button icon - #6395

Merged
AugustinMauroy merged 2 commits into
nodejs:mainfrom
heygsc:main
Mar 2, 2024
ConversationCommits (2)ChecksFiles changed
Merged

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

Conversation

heygsc commented Mar 2, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

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.

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

vercel Bot commented Mar 2, 2024 •
edited
Loading

Copy link
Copy Markdown

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 commented Mar 2, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

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

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

fix: download button icon
18552ab 
Signed-off-by: heygsc <1596920983@qq.com>

heygsc commented Mar 2, 2024

Copy link
Copy Markdown
Contributor Author

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 added the github_actions:pull-request Trigger Pull Request Checks label Mar 2, 2024
github-actions Bot removed the github_actions:pull-request Trigger Pull Request Checks label Mar 2, 2024

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

Copy link
Copy Markdown
Contributor

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 approved these changes Mar 2, 2024
View reviewed changes

AugustinMauroy left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

SGTM ! Thanks for your first contribution 🎉

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

Copy link
Copy Markdown
Contributor

Unit Test Coverage Report

Lines Statements Branches Functions

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 changed the title fix: download button direction fix: download button icon Mar 2, 2024

heygsc commented Mar 2, 2024

Copy link
Copy Markdown
Contributor Author

SGTM ! Thanks for your first contribution 🎉

Thank you!

AugustinMauroy mentioned this pull request Mar 2, 2024
Closed

uncenter commented Mar 2, 2024 •
edited
Loading

Copy link
Copy Markdown

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 commented Mar 2, 2024

Copy link
Copy Markdown
Member

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 commented Mar 2, 2024

Copy link
Copy Markdown

I kinda think on the right makes sense?

ovflowd commented Mar 2, 2024

Copy link
Copy Markdown
Member

I kinda think on the right makes sense?

Why so? (Out of curiosity)

heygsc commented Mar 2, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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 commented Mar 2, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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 commented Mar 2, 2024

Copy link
Copy Markdown

I kinda think on the right makes sense?

Why so? (Out of curiosity)

It just feels right 😅

Right Left

ovflowd commented Mar 2, 2024

Copy link
Copy Markdown
Member

I kinda think on the right makes sense?

Why so? (Out of curiosity)

It just feels right 😅

Right Left

Indeed it looks right. (Ba dum ts)

ovflowd added the fast-track Fast Tracking PRs label Mar 2, 2024

ovflowd commented Mar 2, 2024

Copy link
Copy Markdown
Member

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 added the github_actions:pull-request Trigger Pull Request Checks label Mar 2, 2024
github-actions Bot removed the github_actions:pull-request Trigger Pull Request Checks label Mar 2, 2024
shanpriyan approved these changes Mar 2, 2024
View reviewed changes

shanpriyan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM! Thanks for the PR 🌟

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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

AugustinMauroy AugustinMauroy approved these changes
+1 more reviewer
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

Back | FazBrowse Home | New Git URL