Conversation
|
Sorry I thought I had scaffolded the code better, but we're using styled components for CSS instead of CSS modules (primarily because I figured it would be more straightforward to have everything in one file for people just starting out). For Icons we're using Google's Material Icons (see Figma for all the icons we're using). I didn't have time to import the icons before this week's tasks but Justin imported them in his PR here. I'll share this link in the Slack as well, but here's Google's docs on how to use their icons. |
kohrachel
left a comment
kohrachel
left a comment
There was a problem hiding this comment.
Thanks for the good + quick work!! 💪🏻 great PR description too :)
The component works well, but there seems to be a lot of complexity here + I noticed that the individual menu items don't get selected when I click on them. How about a simpler approach? React has an example dropdown element on their website here.
svgr.d.ts
Outdated
There was a problem hiding this comment.
we're going to be using Google's Material Icons, so we won't need this. This is pretty cool though :)
package.json
Outdated
There was a problem hiding this comment.
we're using styled components instead of tailwind (the tailwind was from the old code, sorry i thought i'd removed it!)
since the Figma is/should define all styles in CSS, i figured it'd be easier to use styled components.
next.config.ts
Outdated
There was a problem hiding this comment.
we'll be importing icons directly from Google so we don't need this, but good work :)
app/components/Dropdown.tsx
Outdated
There was a problem hiding this comment.
we probably shouldn't be passing in onClick into the component (my bad) since the Dropdown component should have some standard logic when an item is clicked
app/components/Dropdown.tsx
Outdated
There was a problem hiding this comment.
typical behavior for a dropdown would be to have the first item be the one that is selected by default, i think that would be a better approach than having a selected property on individual menu items.
app/components/Dropdown.tsx
Outdated
There was a problem hiding this comment.
this feels like it can be simplified.
since the menuItems prop is an array, how about using the array indices to iterate through the array?
app/components/Dropdown.tsx
Outdated
There was a problem hiding this comment.
we should have a state to hold the currently selected item
app/components/Dropdown.tsx
Outdated
| onClick={() => { | ||
| item.onClick?.(); | ||
| closeMenu(); | ||
| }} |
There was a problem hiding this comment.
when an item is clicked, we should set it as the selected item (have some state to store this value)
app/(root)/dropdown-demo/page.tsx
Outdated
There was a problem hiding this comment.
Nice demo! We shouldn't merge demo pages into main but I like it :)
… the list of allowed colors in globals.css
kohrachel
left a comment
kohrachel
left a comment
There was a problem hiding this comment.
Looks good!
Sorry I didn't manage to separate out the text components from this PR. Just made some small nits:
- we're using
bun.lock, so we don't needpackage-lock.json - text components accept an optional color prop, allowed colors are defined in
globals.css - I also just realized that Figma uses Material Symbols Rounded instead of Outlined, so I went in and updated that
Great work!! :)
3 participants
add(component): Dropdown
Added dropdown component. Used Css modules, made style folder, don't know if thats what we're going to do moving forward, let me know if I need to change it. I also added svgr to the project to render svgs as react components, hope thats okay. Also added a dropdown-demo page to test the dropdown.