feat(resources): add ability to choose package manager #173
Conversation
|
Hey @PierreBerger, thanks so much for taking the initiative to add this! Would you left align the options and give a little more spacing? The example in the issue shows the inspiration from |
|
Hey @brookslybrand I've updated the dropdown position, let me know if it's ok for you.
|
|
Hey @PierreBerger sorry, I meant the options should be left aligned too. I do like where you positioned jt
|
|
I hope we are good now 😅 |
There was a problem hiding this comment.
Thanks for the style improvements, this is coming along!
One issue I noticed is that we lost keyboard accessibility unfortunately. You are able to tab to the item and select it, but there's no visual indicator when you're tabbed on it like there was before. Basically when focused the copy button should show up
tabbing-on-resources.mov
Overall I think the code can be cleaned up a good bit. Seems like there was a bit of copy and pasting that might have happened that didn't need to. Also, we want to avoid using the ! tool in tailwind. Everything should be accomplishable via css/regular tailwind classes. If you're not able to figure this part out, that's fine, I plan to keep looking into it
app/ui/resources.tsx
Outdated
| className="!left-auto !right-0 top-10" | ||
| childrenClassName="!w-[110px]" |
There was a problem hiding this comment.
I don't really love how much css with !important we're throwing around, this really shouldn't be necessary. For now you can leave it and make the other suggestions I made and I'll try to look into a better suggestion. Basically you should be able to adjust the parent styles to get this to work I'm pretty sure
|
Hey @PierreBerger, I went ahead and pushed up a number of changes based on what I was trying to get at. Will you look over them and see if they make sense to you? There are a couple of TODOs left in there, if you want you're welcome to take a crack at them. Please don't hesitate to ask if something isn't clear |
brookslybrand
force-pushed
the
feat/resources-package-manager
branch
from
4d67d21
to
c0e9218
Compare
brookslybrand
force-pushed
the
feat/resources-package-manager
branch
from
c0e9218
to
715258c
Compare
|
Alright @PierreBerger, I've gone ahead and cleaned up the a11y issue and some of the css stuff I wanted to be a bit different. Will you review it and let me know what you think? I thin the only thing that remains is adding a test to |
|
Hey @brookslybrand, thanks for the help. I'm sorry, I'm quite busy at the moment. I will work on this next week. |
All good! Really no rush on my part, just wanted to make sure I wasn't blocking you |
This PR adds the ability to select the package manager (npm/yarn/pnpm/bun) when copying installation commands from the resources page.
Maybe I should add a test for the
transformNpmCommandfunction?I've used
DetailsMenucomponent for the Dropdown menu logic.Close #144