-
Notifications
You must be signed in to change notification settings - Fork 23.2k
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
[IMP] website: mega menu and dropdowns on mobile #140083
base: master
Are you sure you want to change the base?
[IMP] website: mega menu and dropdowns on mobile #140083
Conversation
e17f30a
to
7be5df3
Compare
ed959ab
to
02bf907
Compare
02bf907
to
ef2baa2
Compare
b426fa9
to
43e1ee9
Compare
Would be nice to have @Brieuc-brd checking as well since he has a good knowledge of website headers |
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.
Hello @mano-odoo , thanks 👍
I'm just flying-by, some initial remarks for you.
|
||
// Make offcanvas close btn above mega menu | ||
.btn-close { | ||
z-index: 1002; |
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.
Let's hope that no one will never apply 1003
on something else then 😅
Beside jokes, it would be better to avoid arbitrary values.
There is an useful z-index map in bootstrap default, you can use these values but take in account semantic and components type.
Eg. A custom offcanvas that doesn't have the .offacanvas
class may use the $zindex-offcanvas
variable to define its stacking point. Elements that are placed outside (but that should be rendered over it) could use $zindex-offcanvas + 1
.
In order to ease maintenance and readability, everything should use variables.
I guess that these variables should be defined in secondary_variables.scss
... but it's possible that website prefers to keep everything in this file (eventually at the top).
Let's keep this comment open for website clarification ;)
$o_custo_zindex : $zindex-offcanvas;
$o_custo_button_zindex : $o_custo_zindex + 1;
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.
About this approach, I see in website.scss
that we sometimes use variable definition above the selector styling the component.
Note that defining the z-index in secondary_variables.scss
will require me to expose $zindex-offcanvas
since it's not defined yet at this state.
Alternative approaches:
-
Clean way would require me to define a
$o-zindex-offcanvas
in primary_variables + binding it inbootstrap_overriden
it feels a bit overkill since it seems used only for these mega menus for now. -
Compromising by setting the following in secondary_variables:
$o-mega-menu-nav-height: 3rem !default;
$o-mega-menu-zindex: 1045 !default;
$o-mega-menu-btn-close-zindex: $o-mega-menu-zindex + 1 !default;
- Keeping them in
website.scss
where we can make use of$zindex-offcanvas
which make sense to me if we don't plan to use these variables elsewhere than this file.
a6b7e86
to
df96d29
Compare
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.
Nice work @mano-odoo 👍
Some few notes below.
Also, since this PR is related to the Mega menus, could you fix their thumbnails, they seem broken.
c54a2f6
to
8bac007
Compare
4de0293
to
703e7ec
Compare
703e7ec
to
df89d7e
Compare
8b3ee22
to
4ea0a5b
Compare
(conflict solved and rebased) |
@odoo/rd-website little reminder, thanks 🙏 |
@mano-odoo It's in my list of things to check, unfortunately still priority on other things at the moment sorry. |
4ea0a5b
to
b5c3ff5
Compare
@qsm-odoo sorry to ping you again, could you have a look? |
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.
Small review (first two commits), I'll continue tomorrow.
.container > .row { | ||
flex-direction: column; | ||
} |
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.
Does that change anything? In any case, .container
is probably wrong as the user can change that.
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 goal was to make the columns inside the mega_menu take a similar behavior as a "col-12" (full-width column) when it's inside an offcanvas. How is the user able to change the container class? When applying a grid layout in the web_editor it seemed to work fine (sorry I omitted the container-fluid and o_container_small).
Another approach would be to target the elements with the col classes and setting a flex-basis: auto; (again only in offcanvas rendered mega_menu)
@@ -5,10 +5,10 @@ | |||
<section class="s_mega_menu_odoo_menu pt16 o_colored_level o_cc o_cc1"> | |||
<div class="container"> | |||
<div class="row"> | |||
<div class="col-md-6 col-lg pt16 pb24"> | |||
<div class="col pt16 pb24"> |
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 already talked about that before in the team: IMO I think this should either be a global behavior or not a behavior at all. Not a fan of making the col resize different depending on where you are using it 😬 Let's remove that while waiting for a decision. You can create a task about that elsewhere and suggest it later.
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.
Hello, this was done to avoid breaking the mega_menu inside its offcanvas, since we can have the mega_menu displayed in a >lg using the hamburger navigation template it can yield a terrible result. The goal of this col was to provide a consistent result on the mega_menu between offcanvas (large viewport), offcanvas mobile, and normal navigation
The current mobile navigation header is unusable when there are submenus or a mega_menu. This commit improves the usability by implementing a "fake" offcanvas on mega_menu on mobile (essentially a dropdown which behaves visually as an offcanvas). For the submenus, we transform the dropdowns into accordions to improve the navigation experience. Because of the new headers we also have to apply this behavior on hamburger and sidebar header, thus we can't rely on breakpoints to manage the styling since these headers are shown on desktop sized screens. task-3188239
task-3188239
task-3188239
Note: exposing var $-size from website.scss to define the min-width could be worth it. task-3188239
task-3188239
task-3188239
task-3188239
task-3188239
Simplify the gray areas insides the mega menus snippets to fix the weird alignment and remove the extends which are known to increase SCSS compilation time. task-3188239
task-3188239
Since the new offcanvas header have a predefined style, the link-style web_editor option should be disabled to avoid conflicting style. For the sidebar and hamburger menu, the option is disabled in the web editor. But for the other headers this is avoided with a media query in the SCSS to allow link-style on desktop without affecting the offcanvas menu. task-3188239
Fix the broken thumbnails and shorten the code by simplifying shapes and colors. task-3188239
Manage the offcanvas width to display a bit of the page content < sm screens. This allows the user to understand that he is still using a navigation component without being redirected somewhere else. Above sm screens, the offcanvas fallback to a maxwidth of 400px (bs default). This change allows to adapt the line of the offcanvas inside the sidebar template using the new css variable. task-3188239
b5c3ff5
to
302f8bd
Compare
The current mobile navigation header is unusable when there are submenus or a mega_menu. This commit improves the usability by implementing a "fake" offcanvas on mega_menu on mobile (essentially a dropdown which behaves visually as an offcanvas).
For the submenus, we transform the dropdowns into accordions to improve the navigation experience.
Because of the new headers we also have to apply this behavior on hamburger and sidebar headers, thus we can't rely on breakpoints to manage the styling since these headers are shown on desktop sized screens.
Some adaptations were required on the mega menus snippets to better manage their behavior when displayed in the
new offcanvas. To know if they are displayed in an offcanvas scenario the class
o_mega_menu_is_offcanvas
is used as a trigger for the styling.Offcanvas:
On some mobile devices the offcanvas were full screen while others were displaying a bit of the page content behind, sometimes being barely visible. This commit fixes this behavior by always showing the same amount of page content in screens < sm and keeping the max-width behavior on screens above this breakpoint.
Mega menu:
This commit also reworks the gray area extends which were used in the mega menu snippets. It fixes the weird alignment and removes the extends to improve the SCSS compilation time.
The SVGs thumbnails of the mega_menus were broken, this PR seized the opportunity to fix and simplify the code of these SVGs.
task-3188239
I confirm I have signed the CLA and read the PR guidelines at www.odoo.com/submit-pr