-
-
Notifications
You must be signed in to change notification settings - Fork 310
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
[DashboardLayout] Fix sidebar CSS transitions for some breakpoints #4522
base: master
Are you sure you want to change the base?
Conversation
disable the CSS transitions only for the temporary sidebar that shows on mobile devices, but also not needed when CollapsibleSidebar is disabled. Signed-off-by: Gil Obradors <[email protected]>
@@ -240,8 +240,7 @@ function DashboardLayout(props: DashboardLayoutProps) { | |||
[toggleNavigationExpanded], | |||
); | |||
|
|||
const hasDrawerTransitions = | |||
isOverSmViewport && (disableCollapsibleSidebar || !isUnderMdViewport); | |||
const hasDrawerTransitions = isOverSmViewport && !disableCollapsibleSidebar; |
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.
const hasDrawerTransitions = isOverSmViewport && !disableCollapsibleSidebar; | |
const hasDrawerTransitions = isOverSmViewport && (!disableCollapsibleSidebar || isOverMdViewport); |
Thanks a lot for your PR!
Looks like this suggestion would be the correct logic instead, as when disableCollapsibleSidebar
is true
, the non-persistent drawer menu shows up to the md
breakpoint as opposed to just sm
.
This would require replacing the isUnderMdViewport
variable with isOverMdViewport
and inverting all logic where isUnderMdViewport
is currently being used (it's only in a couple of places).
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.
My first suggestion was incorrect, the new one should be good!
Sorry if it caused any confusion.
How hard would it be to add coverage for this in a test? Ideally we aim for adding tests when we fix bugs and especially regressions. |
Probably quite difficult here as it's related to CSS transitions, which could also mean it's not a major issue that needs to be covered by tests... |
No, don't worry in that case. Let's not waste time on it right now. |
Closes #4505
Disable the CSS transitions only for the temporary sidebar that shows on mobile devices, but also not needed when CollapsibleSidebar is disabled.
Enregistrament.de.pantalla.2024-12-08.213424.mp4