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

chore:Added responsive Navbar #240

Merged
merged 2 commits into from
Jan 23, 2022
Merged

Conversation

anirudh-bukka
Copy link
Contributor

Signed-off-by: anirudh-bukka [email protected]

Description

This PR fixes #189

Notes for Reviewers
Added hover effects and corrected the position of the drop down menu list from the hamburger button.

Signed commits

  • Yes, I signed my commits.

Signed-off-by: anirudh-bukka <[email protected]>
@welcome
Copy link

welcome bot commented Jan 22, 2022

Yay, your first pull request! 👍 A contributor will be by to give feedback soon. In the meantime, please review the Layer5 Community Welcome Guide and sure to join the community Slack.
Be sure to double-check that you have signed your commits. Here are instructions for making signing an implicit activity while peforming a commit.

@netlify
Copy link

netlify bot commented Jan 22, 2022

✔️ Website preview ready!

🔨 Explore the source changes: 1f6dd22

🔍 Inspect the deploy log: https://app.netlify.com/sites/getnighthawk/deploys/61ebac81fc78310007453a60

😎 Browse the preview: https://deploy-preview-240--getnighthawk.netlify.app

Copy link
Contributor

@Abhijay007 Abhijay007 left a comment

Choose a reason for hiding this comment

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

@anirudh-bukka great work, it looks good for mobile view, just do the same changes for tab view as well ( max-width: 768px) and in that screen size try to shift nav icon bit towards right

image

you can also take reference from this article if you want to read more about media queries: https://www.geeksforgeeks.org/how-to-target-desktop-tablet-and-mobile-using-media-query/

@anirudh-bukka
Copy link
Contributor Author

@anirudh-bukka great work, it looks good for mobile view, just do the same changes for tab view as well ( max-width: 768px) and in that screen size try to shift nav icon bit towards right

image

you can also take reference from this article if you want to read more about media queries: https://www.geeksforgeeks.org/how-to-target-desktop-tablet-and-mobile-using-media-query/

On it

Signed-off-by: anirudh-bukka <[email protected]>
Copy link
Contributor

@Abhijay007 Abhijay007 left a comment

Choose a reason for hiding this comment

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

LGTM 👍 , great work @anirudh-bukka

Copy link

@YashKamboj YashKamboj left a comment

Choose a reason for hiding this comment

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

@anirudh-bukka Good Work!!! just one small thing , i the "menu" written after the icon is removed ,so if you bring that back would be great .

@anirudh-bukka
Copy link
Contributor Author

anirudh-bukka commented Jan 22, 2022

@anirudh-bukka Good Work!!! just one small thing , i the "menu" written after the icon is removed ,so if you bring that back would be great .

Thanks @YashKamboj ,
When I started working on this issue, the word 'menu' was not present in the forked and cloned files, so I thought the "menu" was not supposed to be there.

But I felt, having the word 'menu' would make the navbar cluttered and congested. What do you say?

@anirudh-bukka
Copy link
Contributor Author

LGTM 👍 , great work @anirudh-bukka

Thanks @Abhijay007 !

@YashKamboj
Copy link

@anirudh-bukka Good Work!!! just one small thing , i the "menu" written after the icon is removed ,so if you bring that back would be great .

Thanks @YashKamboj , When I started working on this issue, the word 'menu' was not present in the forked and cloned files, so I thought the "menu" was not supposed to be there.

But I felt, having the word 'menu' would make the navbar cluttered and congested. What do you say?

I guess it will look great even without menu written

@warunicorn19 warunicorn19 requested a review from debo19 January 22, 2022 17:40
@warunicorn19
Copy link
Member

Might be better toh have just the nighthawk logo in smaller views than the text logo. What do you think @debo19

@adithyaakrishna
Copy link
Member

@anirudh-bukka @YashKamboj @warunicorn19 The UI/UX Convention is not to have a title saying menu as the icon itself stands for hamburger menu so what do you guys think if it can be removed for mobile devices?

@warunicorn19
Copy link
Member

@adithyaakrishna the title menu has already been removed.

@adithyaakrishna
Copy link
Member

My bad xD, I didnt see that :(

@debo19
Copy link
Member

debo19 commented Jan 23, 2022

Might be better toh have just the nighthawk logo in smaller views than the text logo. What do you think @debo19

@warunicorn19 It might look too empty though.🤔 I think this looks fine.

Copy link
Contributor

@Abhijay007 Abhijay007 left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Copy link
Member

@debo19 debo19 left a comment

Choose a reason for hiding this comment

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

LGTM 🎉 Thanks @anirudh-bukka

@debo19 debo19 merged commit cf8acac into layer5io:master Jan 23, 2022
@welcome
Copy link

welcome bot commented Jan 23, 2022

Thanks for your contribution to the Layer5 community! 🎉

Congrats!

@anirudh-bukka
Copy link
Contributor Author

LGTM 👍

Thanks @Abhijay007 for helping me in all the set-up and pushing my first pr!

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

Successfully merging this pull request may close these issues.

Navbar not responsive in any device view
7 participants