-
Notifications
You must be signed in to change notification settings - Fork 163
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
Remove medium breakpoint #4105
Remove medium breakpoint #4105
Conversation
Demo starting at https://vanilla-framework-4105.demos.haus |
@bartaz Lovely work, looks good to me. |
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.
LGTM, just a couple of questions
@@ -34,7 +34,7 @@ Directly inside the drawer you can place one or more navigation components. They | |||
|
|||
On a small screen, the `l-navigation-bar` appears horizontally at the top of the screen. It toggles the side navigation drawer, which is an off-canvas overlay on the left side of the screen. The drawer can be fully collapsed by adding the `is-collapsed` class to the `l-navigation` element. This behaviour can also be invoked using a button or a link in `l-navigation-bar`. | |||
|
|||
On medium or large screen sizes `l-navigation-bar` is not meant to be used and is therefore hidden. Beyond `$breakpoint-medium` the side navigation is always visible on the left side of the screen. | |||
On medium or large screen sizes `l-navigation-bar` is not meant to be used and is therefore hidden. Beyond `$breakpoint-large` the side navigation is always visible on the left side of the screen. |
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.
Only comment I have is on this - it feels like mentioning "medium" screen sizes might cause confusion now that we're not explicitly defining those anymore.
On medium or large screen sizes `l-navigation-bar` is not meant to be used and is therefore hidden. Beyond `$breakpoint-large` the side navigation is always visible on the left side of the screen. | |
On large screen sizes `l-navigation-bar` is not meant to be used and is therefore hidden. Beyond `$breakpoint-large` the side navigation is always visible on the left side of the screen. |
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.
@sowasred2012 We kind of still have a concept of 'medium' screen size. It's a range between small and large breakpoints. At least now we can clearly define it. Before it was always unclear - is medium screen smaller than medium breakpoint or is it larger?...
Anyway, we should update the breakpoints documentation page to what are the breakpoints and what we consider small/medium/large screen.
@@ -34,7 +34,7 @@ Directly inside the drawer you can place one or more navigation components. They | |||
|
|||
On a small screen, the `l-navigation-bar` appears horizontally at the top of the screen. It toggles the side navigation drawer, which is an off-canvas overlay on the left side of the screen. The drawer can be fully collapsed by adding the `is-collapsed` class to the `l-navigation` element. This behaviour can also be invoked using a button or a link in `l-navigation-bar`. | |||
|
|||
On medium or large screen sizes `l-navigation-bar` is not meant to be used and is therefore hidden. Beyond `$breakpoint-medium` the side navigation is always visible on the left side of the screen. | |||
On medium or large screen sizes `l-navigation-bar` is not meant to be used and is therefore hidden. Beyond `$breakpoint-large` the side navigation is always visible on the left side of the screen. | |||
|
|||
On medium screens, the side navigation drawer is by default collapsed to a width of `3rem`. It expands on hover. It can be pinned by adding the `is-pinned` class to the `l-navigation` element. |
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.
Again, the concept of medium screens - not sure what would be better (if you agree this needs to change).
* Remove $breakpoint-medium from the code * Update the examples and docs after removing medium breakpoint
* Remove $breakpoint-medium from the code * Update the examples and docs after removing medium breakpoint
Done
Removed
$breakpoint-medium
from Vanilla and replace it with--large
or--small
breakpoints where relevant.Updated documentation and examples of affected components.
Added
Fixes #4083
QA
Review examples of relevant patterns, make sure they work as expected on different screen sizes:
https://vanilla-framework-4105.demos.haus/docs/examples/layouts/application/default
https://vanilla-framework-4105.demos.haus/docs/examples/patterns/forms/form-inline
https://vanilla-framework-4105.demos.haus/docs/examples/patterns/forms/form-stacked
https://vanilla-framework-4105.demos.haus/docs/examples/patterns/lists/lists-split
https://vanilla-framework-4105.demos.haus/docs/examples/patterns/lists/lists-stepped-detailed
https://vanilla-framework-4105.demos.haus/docs/examples/patterns/matrix
https://vanilla-framework-4105.demos.haus/docs/examples/patterns/tables/table-mobile-card
https://vanilla-framework-4105.demos.haus/docs/examples/templates/responsive
Review updated docs to see if they make sense and don't mention medium breakpoint:
https://vanilla-framework-4105.demos.haus/docs/layouts/application
https://vanilla-framework-4105.demos.haus/docs/patterns/grid
https://vanilla-framework-4105.demos.haus/docs/patterns/lists
https://vanilla-framework-4105.demos.haus/docs/settings/breakpoint-settings
https://vanilla-framework-4105.demos.haus/docs/upgrade-guide-v3