stamford-syntax-club / stamfordcenter-frontend

https://center.stamford.dev/
MIT License
0 stars 0 forks source link

Application navbar #24

Closed NayHtetKyaw closed 1 year ago

NayHtetKyaw commented 1 year ago

it doesn't look good, so pls criticize

chinathaip commented 1 year ago

A non-frontend dev is about to review your code, be prepared!!! XDDD

chinathaip commented 1 year ago

(optional) can you also rebase your branch onto main? (idk if it's just me, but I feel the branches do get messy from git merge compared to git rebase)

Screenshot 2566-08-31 at 17 04 13
Lxkas commented 1 year ago

I don't have my main pc right now xd and still have 3 midterms left, will work on this in a bit

chinathaip commented 1 year ago

I don't have my main pc right now xd and still have 3 midterms left, will work on this in a bit

Maybe work on it and comment it here? Just so we all know how you did it

NayHtetKyaw commented 1 year ago

I don't have my main pc right now xd and still have 3 midterms left, will work on this in a bit

Maybe work on it and comment it here? Just so we all know how you did it

i agreed, i will fix what i can

Lxkas commented 1 year ago

I don't have my main pc right now xd and still have 3 midterms left, will work on this in a bit

Maybe work on it and comment it here? Just so we all know how you did it

i agreed, i will fix what i can

Sure, fix what you can and I will work on it, I got my PC back now 😊

chinathaip commented 1 year ago

I don't have my main pc right now xd and still have 3 midterms left, will work on this in a bit

Maybe work on it and comment it here? Just so we all know how you did it

i agreed, i will fix what i can

Sure, fix what you can and I will work on it, I got my PC back now 😊

Coolio, well done guys @Lxkas @NayHtetKyaw

Keep me updated too

Lxkas commented 1 year ago

pls, like I said, use the NavLink component so you don't have to manage active state manually, along with making a lot of things easier:

👏I beg, use NavLink, you can even remove the onclose callback from the type def, or make it optional

@NayHtetKyaw

NayHtetKyaw commented 1 year ago

pls, like I said, use the NavLink component so you don't have to manage active state manually, along with making a lot of things easier:

* link highlight using pathname from the [router](https://nextjs.org/docs/pages/api-reference/functions/use-router)

* a built-in onclose optional callback

* tab navigation (a11y)

* keyboard interactions, i.e. space for open

* screen-reader support (a11y)

* nested links with a customizable chevron or whatever icon

* auto "list item" styling

* keeping it on-theme

👏I beg, use NavLink, you can even remove the onclose callback from the type def, or make it optional

@NayHtetKyaw

Okay okay, i thought it was optional. I'll try to change it then. I am not familiar with NavLink, that's why i avoided it. !

chinathaip commented 1 year ago

Coolio, well done guys. Thank you Mr Tawan and Anascence. Very excited for this to get merged!

chinathaip commented 1 year ago

closes https://github.com/stamford-syntax-club/stamfordcenter-frontend/issues/30

Lxkas commented 1 year ago

There are a couple of problems now:

Both of these in ApplicationHeader.tsx will have to be re-made so that they're more ergonomic to use, and support multi-level dropdowns.

I've refactored the codebase a bunch, and the mobile navbar is now essentially complete, with multi-level dropdown support, now it's just the desktop stuff that needs another once-over.

Lxkas commented 1 year ago

@NayHtetKyaw also, please tell me why you added react-router-dom ty.

NayHtetKyaw commented 1 year ago

@NayHtetKyaw also, please tell me why you added react-router-dom ty.

we don't need it, i added by accident, though navlink was from react

chinathaip commented 1 year ago

@NayHtetKyaw also, please tell me why you added react-router-dom ty.

we don't need it, i added by accident, though navlink was from react

Aight, I'm gonna remove it and rebase again alright? The commits are all over the place - it'll be a mess when merging back to main @Lxkas @NayHtetKyaw

NayHtetKyaw commented 1 year ago

so i guess the mobile navbar is done now ?

Lxkas commented 1 year ago

so i guess the mobile navbar is done now ?

Yes, the mobile navigation is done but it also broke the normal navigation on desktop, when it's fixed, this can be merged 👍