-
Notifications
You must be signed in to change notification settings - Fork 2
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
Cgd 33 navbar #11
Cgd 33 navbar #11
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Oh, I actually already made a reusable Avatar component in my pr. I guess we all had avatars in our ticket lol |
Ah, ok. That's cool. It looks like they're pretty similar components in terms of logic, we'll get yours merged and then I'll just reconfigure the nav to use that Avatar 🙂 |
Another thing I noticed is you could've used the dropdown from daisy ui here: https://daisyui.com/components/dropdown/#dropdown-on-hover (as an example). Is there an issue with using this? would clean up all the extra logic and markup / classes you added. |
That is actually just using the JSX from the DaisyUI, the reason for adding in the extra logic is that the open and close functionality doesn't exist natively for DaisyUI, like click away/outside to close, or clicking on a link to close won't work as there's no JS, so it's unlike a full UI library like MUI or something. The same goes for modals in DaisyUI, we'll need to use React refs to close the modals with the esc key and trap tab key movements (i.e. focus traps for accessibility purposes), or click outside of modal to close or however we choose to do that. DaisyUI is lightweight and resultantly not full featured. I think it's better to use a common hooks library that takes care of the logic for us and which we can import and reuse elsewhere with a layer of abstraction rather than us each or one person creating a click/focus trap or something like that as a utils function. |
It does work though? All of that is built in, I tested it locally and it works. I'm not sure I fully understand what you're saying here. |
Ah, ok they have some options which recognise clicking outside of the element now, when I'd used them in the past they hadn't, I took the Nav from a component they had and it didn't close with clicks outside so I just presumed it was the same story. It still won't work for action on the same page, like clicking an I've refactored it to use the native DaisyUI dropdown btn, which is fine for now. Also removed the useHooks dependency. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
• Add Navbar component and children components
• Refactor nested ternary in TechStack component to use a function with standard control flow to improve readbility and ESLint issues with spacing.
• Refactor the Avatar component to be reusable, with default settings applied (Have justed added in the grey ball .png for the time being as I assume the grey sections for the techstack are just temporary while they're being developed? Not idea though, we could perhaps change this out or rename that for another component, would make sense to me that an avatar be relating to a use picture like that found in the Navbar, which is how I've refactored it to look.)