Skip to content
This repository has been archived by the owner on Aug 7, 2024. It is now read-only.

fix: secondary navbar overflow (#9891) #10023

Merged
merged 5 commits into from
Jan 21, 2024

Conversation

ankittankal
Copy link
Contributor

@ankittankal ankittankal commented Dec 25, 2023

Fixes Issue

closes #9891

Changes proposed

Check List (Check all the applicable boxes)

  • My code follows the code style of this project.
  • My change requires changes to the documentation.
  • I have updated the documentation accordingly.
  • All new and existing tests passed.
  • This PR does not contain plagiarized content.
  • The title of my pull request is a short description of the requested changes.

Screenshots

Screenshot_20231225_155413

Note to reviewers

I corrected the issue by setting a custom 900px breakpoint specifically for tablets, as the mobile view is intended for smaller screens and not for large screens(1024px).

@ankittankal ankittankal marked this pull request as ready for review December 25, 2023 10:35
@ankittankal
Copy link
Contributor Author

Could anyone provide a review on my pull request, please?

@eddiejaoude
Copy link
Member

Sorry for the delay, reviewing it now

Copy link
Member

@eddiejaoude eddiejaoude left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution

I don't think we should introduce a new screen size because of the content, we should adjust the content.

my thoughts for small screens

  • main navigation: hide theme toggle, GitHub icon and profile picture
  • tabs: switch to drop down nav sooner

@eddiejaoude
Copy link
Member

eddiejaoude commented Jan 15, 2024

@ankittankal why did you request a review, I gave a review and feedback a few hours ago and there are no new changes or comments?

@ankittankal
Copy link
Contributor Author

@ankittankal why did you request a review, I gave a review and feedback a few hours ago and there are no new changes or comments?

I appreciate your initial review and feedback. Apologies for the accidental re-review request; it was unintentional. I'll ensure to avoid any confusion in the future.

@ankittankal
Copy link
Contributor Author

hey Eddie, first point is already what happens, git, theme and profile icon are hide in smaller screen.

and the second point is what I have done in new changes, I introduced a screen because the next breakpoint after sm is md which would be too soon to start hiding icons.

@eddiejaoude
Copy link
Member

I introduced a screen because the next breakpoint after sm is md which would be too soon to start hiding icons.

I am keen to keep things simple and this will have a knock on effect for other pages. To keep things simple for now I think we should stick to the original break points and make other decisions accordingly - I don't see hiding things earlier as a problem if there is no space

Copy link
Member

@eddiejaoude eddiejaoude left a comment

Choose a reason for hiding this comment

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

Thank you 👍

@eddiejaoude eddiejaoude merged commit 6583729 into EddieHubCommunity:main Jan 21, 2024
13 checks passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
issue linked Pull Request has issue linked
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Secondary Navbar Overflow Issue Between 630px to 740px Screen Width
3 participants