-
Notifications
You must be signed in to change notification settings - Fork 126
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
Updated/Standardized navbar of member-site #265
Conversation
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.
- Can you please remove the
Home
tab and add theStatus
site tab in the nav bar. You can refer to the screenshot in the issue description ?? - On clicking the logo, we should be directed to
Home
Page - Add the screenshot of the mobile view as well.
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.
@Kshashank99 In the mobile view, please add the link of status
site also and if possible can you please left-align X
a bit.
Removed comment
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.
Good work👍, resolve the last NITs please
Hey @rohan09-raj, @sumitd94 and @harshith-venkatesh Please review the PR. |
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.
@Kshashank99 Can you please remove all the previous attached images in conversation and only upload the current updated UI screenshot in the description. It is bit confusing for us to understand which is the latest one.
Upload the following screenshots :
- Desktop View
- Mobile view (navbar menu closed)
- Mobile view (navbar menu open)
@rohan09-raj @sumitd94 @swarajpure Please review the PR #265 : Before:Desktop viewMobile viewAfter:Desktop viewMobile view |
src/components/UI/navbar/index.js
Outdated
<li className={nav.name === 'Home' ? `${styles.Homes}` : null}> | ||
<li | ||
className={nav.name === 'Home' ? `${styles.homes}` : null} | ||
key |
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.
Why this prop has no value ??
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.
Added Id
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.
Looks good to me 💯 , thanks for being patient and actively addressing comments. Really appreciate the efforts. Approved from my side
setUserData({ | ||
userName: responseJson.username, | ||
firstName: responseJson.first_name, | ||
profilePicture: responseJson.picture.url?.DEFAULT_AVATAR, |
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.
Nice 💯
I have made the changes as per the instructions. Please review my PR.
![image](https://user-images.githubusercontent.com/78433013/150916963-a86f0364-fe92-4d35-9040-ef6621cb360e.png)
![image](https://user-images.githubusercontent.com/78433013/150917020-83efb597-5ee2-42c8-b5e4-91fbcada9531.png)
Updated Navbar :
Closes #261