-
Notifications
You must be signed in to change notification settings - Fork 48
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
Increased length of clickable space in navbar #566
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@diamondburned This was an issue you pointed out so let me know what you think about this solution! |
Fixes #511 as well |
I prefer the old dark navbar better with the dark theme, but I think the new changes are overall more consistent with the design. |
I'll try to actually change it back to the dark theme navbar but add perhaps a shadow. |
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.
this navbar need refactor tbh
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.
- acm icon is above overlay
- backdrop animation is too slow
- mobile navbar is too tall
Snapchat-1859405296.mp4
Snapchat-420092908.mp4
Screen_Recording_20221011_125415_Samsung.Internet.mp4possible to disable scrolling here too? |
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.
button not seem to work here
Screen_Recording_20221011_125627_Samsung.Internet.mp4
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.
your contribution improves the mobile experience.
we are, however, starting to realize the technical debt with the current navbar structure. Perhaps a refactor is needed.
we may also want to package up some of these frequently used css properties like:
position: absolute;
width: 100%;
height: 100%;
into their own class to reduce code bloat/smell.
we may also want to reduce the css nesting levels. i am not sure on the best approach but some of these elements are nested 5-6 levels deep and all within a media query as well. unmaintainable!!! (separate issue to be solved alongside the html refactor prob)
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.
- logo z index wrong again
- overlay transition seem to not correct
Snapchat-1135243973.mp4
When you get a chance, please take a look at this @jaasonw! Thank you! |
the reason it's taking so long is cus im still looking into fixing the exit animation and havent had the time |
We can make it a separate issue if you would like, but if not I will also look into it as well. Thank you for letting me know. |
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.
fixed
Previously, in mobile mode, the size of the clickable area in the navbar was significantly small so it required you to press the names of the pages directly rather than the surrounding area itself.
Now it has been updated to encompass a larger area of clickable space. The size should be from one end of the screen to the other end.
I am open to suggestions if whether we should have a title for the navbar, such as like "Menu", so that I can push down all the pages and make it easier to click if they are all situated near the bottom. I am also unsure of how to fix the words stacking on top of each other in "Node Buds". I am aware we have removed that page from the navbar, but for future pages it should not be stacked.
Fixes #279.
This PR additionally makes the navbar transparent, which addresses #580.