feat: Added Navbar component

[Rounak-stha]

Description

This PR creates Navbarcomponent. The figma design is followed and the component is tested visually.

Validation

Related Issues

Addresses #6060

Check List

• I have read the Contributing Guidelines and made commit messages that follow the guideline.
• I have run npx turbo lint to ensure the code follows the style guide. And run npx turbo lint:fix to fix the style errors if necessary.
• I have run npx turbo format to ensure the code follows the style guide.
• I have run npx turbo test to check if all tests are passing.
• I've covered new added functionality with unit tests if necessary.

[vercel]

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

Name	Status	Preview	Comments	Updated (UTC)
nodejs-org	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Nov 15, 2023 2:34pm

[Rounak-stha]

Here are some of my concerns

• It is mentioned in the issue to add the key components.headers.links.learn in the en.json file has a key components.header.links.learn. Notice the header(s) in the key mentioned in the issue
• Currently, the navbar has seven items, but in the new design, only six are used; the Get Involved nav item is removed. The current navigation.json file has all seven items and like in the current implementation of Header, I've used the useNavigation hook but exported new items from it to only get items listed in the new design.
• The navigation item text for blog is blog in design file but the formattedMessage is news
• I didn't see the nav items design for mobile. The design hides the nav items and has added a Hambuger icon; how should the items be shown on open state on mobile.

[github-actions]

Lighthouse Results

URL	Performance	Accessibility	Best Practices	SEO	Report
/en	🟢 100	🟢 97	🟠 83	🟢 92	🔗
/en/about	🟢 100	🟢 95	🟠 83	🟢 92	🔗
/en/about/previous-releases	🟢 100	🟢 96	🟠 83	🟢 92	🔗
/en/download	🟢 100	🟢 97	🟠 83	🟢 92	🔗
/en/blog	🟢 98	🟢 97	🟠 83	🟢 92	🔗

[github-actions]

Unit Test Coverage Report

Lines	Statements	Branches	Functions
	79.34% (361/455)	64.81% (70/108)	63.04% (58/92)

Unit Test Report

Tests	Skipped	Failures	Errors	Time
54	0 💤	0 ❌	0 🔥	4.745s ⏱️

[Rounak-stha]

Hmmm @Rounak-stha I'm not seeing the styles of active navigation item here 🤔

TBH, this was bugging me too. As I didn't see the style for the active item in the design file like in the mobile view, I skipped to add it.
I'll be adding this and changing the cursor to pointer on Label too.

[ovflowd]

I can see them working on mobile (@Rounak-stha) but with wrong text colour on light theme 👀

Yes, I've mentioned this
The NavItem component has set its own styles for the text color and we can't change that frok the nav component. May be we need to expose a additional prop on the NabItem component to control the text color of the active item.

I'd say the NavItem styles are wrong then, as active item with light theme should have white text. I think you can fix these styles directly on NavItem component :)

[Rounak-stha]

I'd say the NavItem styles are wrong then, as active item with light theme should have white text. I think you can fix these styles directly on NavItem component :)

I went through the NavItem component in detail and found that it does have styles for active navItem which is passed to the ActiveLocalizedLink component which uses the usePathname hook. Because we're viewing in storyboook, the pathname does not actually match. The pathname is null as the url looks like this: http://localhost:6006/?path=/story/containers-navigationbar--default&globals=theme:dark.

[ovflowd]

I'd say the NavItem styles are wrong then, as active item with light theme should have white text. I think you can fix these styles directly on NavItem component :)

I went through the NavItem component in detail and found that it does have styles for active navItem which is passed to the ActiveLocalizedLink component which uses the usePathname hook. Because we're viewing in storyboook, the pathname does not actually match. The pathname is null as the url looks like this: http://localhost:6006/?path=/story/containers-navigationbar--default&globals=theme:dark.

But why on the Mobile View Port it works? That's what bugs me k

[ovflowd]

Also another bug, if I change viewport while the mobile menu was open... This happens:

I think this is edge case, as well viewports doesn't change that drastically, but probably something we should 👀

[ovflowd]

Also @Rounak-stha can you check why ActiveLocalizedLink is not working? I think there might be a bug there (I had to change the logic there... The unit tests are passing but they are weird and I don't even know why they are passing 😅

[Rounak-stha]

But why on the Mobile View Port it works? That's what bugs me k

Because I am adding those styles from the NavigationBar component (as in design file; right now looks like I kinda forced it)

[ovflowd]

But why on the Mobile View Port it works? That's what bugs me k

Because I am adding those styles from the NavigationBar component (as in design file; right now looks like I kinda forced it)

Hmm, I think we shouldn't force those styles (active styles) IMO, are they really needed? The pathname checks for ActiveLocalizedLink are fine, and I feel that's how we should also check.

I think we can remove the currentNavItem prop, and just let ActiveLocalizedLink do its job. Otherwise we would need to invoke the same logic anyways to the component rendering NavigationBar

[Rounak-stha]

As the ActiveLocalizedLink component uses the usePathName hook to check if the NavItem is the current item, this does not play well with storybook. I used the NavigationBar component on the actual web page and the result looks like this:

[ovflowd]

As the ActiveLocalizedLink component uses the usePathName hook to check if the NavItem is the current item, this does not play well with storybook. I used the NavigationBar component on the actual web page and the result looks like this:

So on actual usages it works well?

I wonder if we can mock next router on storybook so that it returns the path that we want.

If you feel like spending tile investigating on that, that's an extra, as it'd be cool to test the active item happy path on storybook (mostly to see if the designs look well)

Otherwise, let it be

[Rounak-stha]

If you feel like spending tile investigating on that, that's an extra, as it'd be cool to test the active item happy path on storybook (mostly to see if the designs look well)

Sure. I'll take a look at mocking that data

[ovflowd]

Just don't forget about this (@Rounak-stha) > #6072 (comment) (didn't see you giving it a 👍 so maybe you missed that one)

[ovflowd]

@Rounak-stha I was able to find the bug with ActiveLocalizedLink on storybook and pushed a fix on main. usePathname was returning "null"...

So no need for more investigations on your side.

[Rounak-stha]

@Rounak-stha I was able to find the bug with ActiveLocalizedLink on storybook and pushed a fix on main. usePathname was returning "null"...

Yeap, I checked. In the edfault SB state, the usePathname returns / which is not on the list of our navItems. I changed the href of oneof the items to get this:

The SB nextjs addon should handle the Link component navigation, shouldn't it?

[ovflowd]

The SB nextjs addon should handle the Link component navigation, shouldn't it?

wdym?

Yeap, I checked. In the edfault SB state, the usePathname returns / which is not on the list of our navItems. I changed the href of oneof the items to get this:

Yeah, that's how I tested. I had to update the Storybook settings to "enable" the App Router functionality. So working fine now :)

[ovflowd]

Honestly speaking, I think there's nothing else blocking us and everything seems to be working!

So let's get this merged 🎉 I don't think I've missed anything else and all styles seem correct :)

We can leave the "mock" of usePathname for another PR.