Skip to content
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

Revamp Navbar #196

Merged
merged 6 commits into from
Oct 16, 2022
Merged

Revamp Navbar #196

merged 6 commits into from
Oct 16, 2022

Conversation

Keegan-20
Copy link
Contributor

Related Issue

Proposed Changes

  • Change 1:changed the font-weight in the anchor tag
  • Change 2:changed the hover Effect in Navbar anchor tags
  • Change 3: The most important one added social media icons with a dropdown menu along with making them responsive

Checklist

  • [ x] Documentation of the code
  • [ x] Add your name to the CONTRIBUTORS file
  • [x
    girlscript op
    ] Testing your code in the local machine

Screenshots

@netlify
Copy link

netlify bot commented Oct 13, 2022

Deploy Preview for girlscript-asansol ready!

Name Link
🔨 Latest commit fae2795
🔍 Latest deploy log https://app.netlify.com/sites/girlscript-asansol/deploys/634c0aa631798d0007cf5bcc
😎 Deploy Preview https://deploy-preview-196--girlscript-asansol.netlify.app/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Greetings from GS Asansol! Thanks for making the PR, our team will soon review your PR. Thanks :)

@Keegan-20
Copy link
Contributor Author

Hello i have done necessary changes and kindly revert back if any changes else accept the merge request.

Thank You

@jayzsh
Copy link
Member

jayzsh commented Oct 14, 2022

@theanimator20 Sorry for the delay. We've been a little busy. We'll review and merge it by midnight.

Copy link
Contributor

@Debdeep1 Debdeep1 left a comment

Choose a reason for hiding this comment

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

I think the navbar in the mobile view will require a lot of adjustments. The alignment is off at places. Do you wanna try this again? Or you can always let us reassign this to someone else.

@Keegan-20
Copy link
Contributor Author

Yh i can work on it just tell me how u want the adjustments in mobile view

@jayzsh
Copy link
Member

jayzsh commented Oct 14, 2022

Desktop view:

  • GSA icon is way too shifted towards the left. You could provide some padding.
  • The Navbar hyperlinks look way too faded, increase the opacity a little bit maybe? Could we also have a smooth transition to bright, and the way back?
  • The dark/light theme button would look better aligned to the right end.
  • Can you make the dropdown look simpler? I'm attaching a sample. All that the dropdown rows should contain are the social media icons and the service handles.

Sample

Mobile view:

  • You could try to make this tray change colour depending on the theme chosen. This could be optional tho. However, try aligning the logos properly one after the other with top and bottom padding for the first and the last element in the list respectively. Also try adding some corner radius.

  • The same issue with the faded font fill. Also, the items are aligned to the right.

Copy link
Member

@jayzsh jayzsh left a comment

Choose a reason for hiding this comment

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

@theanimator20

@Keegan-20
Copy link
Contributor Author

Ok i will work on it but the light/theme button is at top-right only na?. Also for social link I'll remove the margin-top and make it kind of similar to that

@Keegan-20
Copy link
Contributor Author

@jay-io i have worked on the issues u asked for do check it out

@jayzsh
Copy link
Member

jayzsh commented Oct 15, 2022

Screenshot (16)

@theanimator20 I'm not certain about mobile view tho. Can you still try to correct the overlaps and the right shift?

@Keegan-20
Copy link
Contributor Author

Keegan-20 commented Oct 15, 2022

@jay-io is it possible to accept the PR
for WHATEVER I worked till now since i have other projects as well to work on and handle this work to others?.

@jayzsh
Copy link
Member

jayzsh commented Oct 15, 2022

@theanimator20 We really appreciate your efforts. That's not all. We also love your follow-us mobile-view-only sidebar.

Can you revert all the changes you did on the nav panel? We'll have someone else look into it. A few other contributors approached us and they wish to give it a shot.

If you can still do it, you're most welcome to try and seek help in our server. If it's been tiresome for you, I'm afraid we can't merge a PR that kinda breaks stuff, just to help contributors win their T-shirts. We have milestones to reach, too.

It's your call now.

@Keegan-20
Copy link
Contributor Author

Ok i will try to work on overlapping issue if u need other changes do let me know once again

@jayzsh
Copy link
Member

jayzsh commented Oct 15, 2022

Ah, yes the slight right-shift that the mobile navbar options have got. If you compare what we have currently deployed on our main branch and your mobile navbar, you'll know exactly what I'm talking about.

Btw, glow effect is nice too, see if you can preserve it.

@Keegan-20
Copy link
Contributor Author

Okay i would work on it

@Keegan-20
Copy link
Contributor Author

@jay-io changes Resolved Check.
mop

@soumali28 soumali28 merged commit 249d657 into gsasansol:main Oct 16, 2022
@jayzsh
Copy link
Member

jayzsh commented Oct 16, 2022

Looks great now, thank you for contributing!

@Keegan-20
Copy link
Contributor Author

@jay-io Thanks for having me here and i learned alot of new things and also thanks for Guiding Me.
Happy coding!!

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

Successfully merging this pull request may close these issues.

None yet

4 participants