Skip to content

[Frontend] Standardize the Navbar - #264

Closed
rohan09-raj wants to merge 9 commits into
developfrom
feature/nav-bar
Closed

[Frontend] Standardize the Navbar#264
rohan09-raj wants to merge 9 commits into
developfrom
feature/nav-bar

Conversation

@rohan09-raj

@rohan09-raj rohan09-raj commented Jun 13, 2022

Copy link
Copy Markdown
Contributor

Background

The nav-bars across all sites are not consistent and so we have tasks to make it consistent on all sites.

Implementation

This PR deals with template and styling changes for the navbar on the status site.
Closes #233
Referenced CSS from RealDevSquad/website-crypto#241

Visual Changes

Before :
image
image

After :
image
image

Comment thread src/components/navBar/index.tsx Outdated
Comment thread src/components/navBar/index.tsx Outdated
Comment thread src/components/navBar/index.tsx Outdated
Comment thread src/components/navBar/index.tsx Outdated
Comment thread src/components/navBar/index.tsx Outdated
Comment thread src/components/navBar/navBar.module.scss Outdated
Comment thread src/components/navBar/navBar.module.scss Outdated
Comment thread src/components/navBar/navBar.module.scss Outdated
Comment thread src/components/navBar/navBar.module.scss Outdated
Comment thread src/components/navBar/navBar.module.scss Outdated
Comment thread src/components/navBar/index.tsx Outdated
@Neha

Neha commented Jun 13, 2022

Copy link
Copy Markdown

SCSS file needs complete re-write. We are not utilizing the power of SCSS in this at all. If we are using SCSS then we should get 100% out of it.

Comment thread src/components/navBar/navBar.module.scss Outdated
Comment thread src/components/navBar/index.tsx Outdated

@Neha Neha left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job!! 👍 A few more improvements and we will be good to merge this. Yay!!

Comment thread src/components/navBar/index.tsx Outdated
Comment thread src/components/navBar/index.tsx Outdated
Comment thread src/components/navBar/index.tsx Outdated
Comment thread src/components/navBar/index.tsx Outdated
Comment thread src/components/navBar/index.tsx
Comment thread src/components/navBar/navBar.module.scss Outdated
Comment thread src/components/navBar/navBar.module.scss Outdated
Comment thread src/components/navBar/navLinks.ts Outdated
@Neha

Neha commented Jun 21, 2022

Copy link
Copy Markdown

Question: why we have 2 components here (UserSignIn)
image

Suggestion: Instead of updating the state in the local component move it to context API

@rohan09-raj

rohan09-raj commented Jun 22, 2022

Copy link
Copy Markdown
Contributor Author

Question: why do we have 2 components here (UserSignIn)

We have two components because of the responsiveness issue therefore when we are in the desktop mode we have a single <ul> inside which we have all the links and sign-in button (first use of UserSignIn component inside <ul>) but when we are in mobile mode if we use that sign-in component it would be somewhere here as shown in below image
image
And we cannot move that component outside of the <ul> container with SCSS or CSS. That's why we need one more component.
I guess we can avoid this if we use the component which is outside in <div>.
image

UPDATE : Fixed the issue of repeated components

I guess using context API here would not make sense because we are not nesting the components by 2 or 3 or more layers, we are just using 1 layer for passing the data and that won't change until we change the design of the navbar and add some more logic and nesting to this navbar component.

@Neha

Neha commented Jun 23, 2022

Copy link
Copy Markdown

Question: why do we have 2 components here (UserSignIn)

We have two components because of the responsiveness issue therefore when we are in the desktop mode we have a single <ul> inside which we have all the links and sign-in button (first use of UserSignIn component inside <ul>) but when we are in mobile mode if we use that sign-in component it would be somewhere here as shown in below image image And we cannot move that component outside of the <ul> container with SCSS or CSS. That's why we need one more component. I guess we can avoid this if we use the component which is outside in <div>. image

UPDATE : Fixed the issue of repeated components

I guess using context API here would not make sense because we are not nesting the components by 2 or 3 or more layers, we are just using 1 layer for passing the data and that won't change until we change the design of the navbar and add some more logic and nesting to this navbar component.

Well, this is not a scalable solution. If tomorrow we wanted to use user details somewhere else then it won't scale. As well as, details such as user details ( username , and profile picture ) should be move up to the global. So, that it is available across the app.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Standardize the Navbar

5 participants