-
Notifications
You must be signed in to change notification settings - Fork 176
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
frontend: Fix visibility of Sidebar Elements and Navigation Tabs #1720
frontend: Fix visibility of Sidebar Elements and Navigation Tabs #1720
Conversation
Signed-off-by: Mariusz Winnik <[email protected]>
ea4c0dd
to
63c60b4
Compare
…topbar-mobile-fixes
Signed-off-by: Mariusz Winnik <[email protected]>
830a907
to
bba6366
Compare
Signed-off-by: Mariusz Winnik <[email protected]>
bdbd6b5
to
e500d42
Compare
Hey @tazo90 , thanks for this PR. I have noticed that the logo looks broken with it though (maybe due to the updates we had in the meanwhile): I will review the code now. |
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.
Thanks again @tazo90 . I left some comments on how to improve this PR.
@@ -31,6 +31,7 @@ import VersionButton from './VersionButton'; | |||
export const drawerWidth = 240; | |||
export const mobileDrawerWidth = 320; | |||
export const drawerWidthClosed = 64; | |||
export const topBarHeight = 64; |
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.
The problem is that the sidebar is set from the top of the screen, without having the toolbar before it.
A more future-proof way of fixing this issue is to see what the normal case is doing: Adding a Box with the toolbar style mixin (look for the use of the toolbar
class).
frontend/src/components/App/Home/__snapshots__/RecentClusters.stories.storyshot
Show resolved
Hide resolved
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.
We no longer have the frequent
namespace. Only glossary + translation (default).
@tazo90 , have you had any more time to work on this? Really excited for this PR to get in so that headlamp is usable on mobile for me |
@tazo90 , if you don't have time to work on this PR, we understand and we can take it from here. Please let us know. |
Closing in favor of #2062 . |
This PR fixes #1719 .
Problem 1: Sidebar elements not visible
Before:
After:
Problem 2: Invisible Navigation Tabs
Before:
After: