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: install button with dropdown for npm, pnpm, yarn and bun #10504

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

mathisdev7
Copy link

This PR modifies a documentation component to enhance the install button functionality. The dropdown now includes npm, pnpm, yarn, and bun, allowing users to easily copy the installation command for their preferred package manager. This update improves the flexibility and user experience for developers consulting the documentation.

Status and versioning classification:

@mathisdev7 mathisdev7 requested a review from a team as a code owner September 14, 2024 06:43
Copy link

vercel bot commented Sep 14, 2024

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

Name Status Preview Comments Updated (UTC)
discord-js ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 14, 2024 10:50am
1 Skipped Deployment
Name Status Preview Comments Updated (UTC)
discord-js-guide ⬜️ Ignored (Inspect) Visit Preview Sep 14, 2024 10:50am

@mathisdev7 mathisdev7 changed the title Added install button with dropdown for npm, pnpm, yarn and bun feat: install button with dropdown for npm, pnpm, yarn and bun Sep 14, 2024
onClick={() => setShowDropdown(!showDropdown)}
type="button"
>
<span className="font-semibold text-blurple">{'>'}</span> npm install discord.js
Copy link
Member

Choose a reason for hiding this comment

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

This makes it look really weird, npm is always shown but dropdown select makes it copy a diff package manager

Copy link

codecov bot commented Sep 14, 2024

Codecov Report

Attention: Patch coverage is 0% with 55 lines in your changes missing coverage. Please review.

Project coverage is 35.37%. Comparing base (495bc60) to head (b8afd59).

Files with missing lines Patch % Lines
apps/website/src/components/ui/InstallButton.tsx 0.00% 55 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #10504      +/-   ##
==========================================
- Coverage   35.48%   35.37%   -0.11%     
==========================================
  Files         228      228              
  Lines       14308    14351      +43     
  Branches     1254     1254              
==========================================
  Hits         5077     5077              
- Misses       9187     9230      +43     
  Partials       44       44              
Flag Coverage Δ
website 0.00% <0.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mathisdev7
Copy link
Author

Reviewed the PR, can you check if it is good please, thanks.

Copy link
Member

@almeidx almeidx left a comment

Choose a reason for hiding this comment

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

From a usability standpoint, doesn't make much sense that for npm, I have to open the select and then select npm (which was already selected) for it to copy the command. Perhaps we should rethink the way this works.

We could keep the button as it was before to copy the command, and have a menu to the side which selects the package manager?

Comment on lines +7 to +18
const getCommand = (manager: string) => {
switch (manager) {
case 'pnpm':
return 'pnpm install discord.js';
case 'yarn':
return 'yarn add discord.js';
case 'bun':
return 'bun add discord.js';
default:
return 'npm install discord.js';
}
};
Copy link
Member

Choose a reason for hiding this comment

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

You could just make an array of { packageManager: string; command: string } or something like that, and then loop over that to generate the list items


{showDropdown && (
<div className="absolute left-0 mt-2 w-full rounded-md border border-neutral-300 bg-white shadow-lg dark:border-neutral-700 dark:bg-neutral-800">
<ul className="py-1">
Copy link
Member

@almeidx almeidx Sep 14, 2024

Choose a reason for hiding this comment

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

Maybe use Menu from react-aria-components instead (or another one from that lib, if you believe it fits the purpose better)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Review in Progress
Development

Successfully merging this pull request may close these issues.

3 participants