OEL-4185: Improved mega menu. #702

tibi2303 · 2025-10-30T13:12:11Z

No description provided.

github-actions · 2025-10-30T13:17:24Z

🚀 Deployed on https://preview-702--oelibrary.netlify.app

donquixote

I am submitting the review now so it is not lost.
@tibi2303 already told me he is working on some changes.

donquixote · 2025-11-03T16:07:44Z

src/compositions/bcl-header/header.html.twig

-    </div>
-  </div>
-  {% endif %}
-


Does this removal have any impact on websites?
-> I think not, there is no pattern for header.

However, I assume that we will do a similar change in oe_whitelabel.

It seems the goal is to remove stuff from the <header> element that would otherwise conflict with the CSS.

no, the pattern is not used.. i just make it somehow closer to what we have now on oesa

donquixote · 2025-11-03T16:17:52Z

src/components/bcl-navbar/__snapshots__/navbar.test.js.snap

                            <div
                              aria-labelledby="custom-trigger-mm-1"
                              class="bcl-mega-menu__submenu __level-2"
+                              hidden="true"


This looks wrong. The hidden attribute should not have a value, at least not in its default usage.
https://developer.mozilla.org/en-US/docs/Web/HTML/Reference/Global_attributes/hidden
(see below, I will also comment in the respective template)

fixed it by extending the drupal attributes, have a look now.. multiple test snapshots are updated because of this

in test snapshot are like this: hidden="" but in html of storybook is just hidden

donquixote · 2025-11-03T16:19:59Z

src/compositions/bcl-mega-menu/mega-menu-submenu.html.twig

 {% set submenu_attributes = create_attribute()
  .setAttribute('id', _panel_id)
-  .setAttribute('hidden', '')
+  .setAttribute('hidden', 'true')


The effect of this seems wrong, I see hidden="true" in the snapshots.

I think .setAttribute() is different tech in Drupal vs BCL twig, and I am not sure what is the correct way to set a value-less attribute. Maybe '' works in Drupal but not here? That would be bad.

donquixote · 2025-11-03T16:24:00Z

tools/webpack/twig-wrapper-loader.cjs

+  return Promise.resolve(resolved);
+};`,
+  );
+


This looks magical.
Can we add a comment explaining what we are doing here?
If you are not sure what to write, start by explaining it to me (here or in chat).

i added the comments to be more clear

donquixote · 2025-11-18T20:34:03Z

src/themes/default/src/scss/_mega-menu.scss

  margin: 0;
  padding-inline-start: 0;
  // Add a scrollbar in desktop viewport.
+  .see-all-button {


I don't agree with this selector hierarchy ul.bcl-mega-menu__items .see-all-button.
Yes, the .see-all-button happens to be inside of a .bcl-mega-menu__items, but that is actually the item list of the parent level. Too far removed.

More logical would be .bcl-mega-menu__submenu > .__body > .__see_all > .see-all-button, or .bcl-mega-menu__submenu > .__body > .__see_all > a or just .bcl-mega-menu__submenu .see-all-button.

Or there could be a dedicated namespaced class instead of .__see-all, so we could do .bcl-mega-menu__submenu-see-all > a.

src/themes/default/src/scss/_mega-menu.scss

donquixote · 2025-11-18T20:44:08Z

src/themes/default/src/scss/_mega-menu.scss

+  }
+  @include media-breakpoint-up(lg) {
+    > .__body {
+      background: $primary-bg-subtle;


This is not needed, the parent already has this background.

donquixote · 2025-11-18T20:50:57Z

src/compositions/bcl-mega-menu/mega-menu-submenu.html.twig

-    icon_path: _icon_path,
-    level: _level,
-  } only %}
+  <div class="__body">


Is this __body wrapper actually necessary?
It seems that it would be simpler without.

donquixote · 2025-11-18T21:00:38Z

src/themes/default/src/scss/_mega-menu.scss

+  .see-all-button {
+    @include mm-item-padding();
+    // Use flex for icon spacing.
+    display: flex;


Note:
This used to have justify-content: space-between, which pushed the icon to the right.
By dropping that rule, we align closer with Figma.
-> good :)

donquixote

@tibi2303 I left some comments but also pushed some commits.
Let me know if you agree with these.

src/themes/default/src/scss/_mega-menu.scss

donquixote · 2025-11-18T22:33:14Z

src/themes/default/src/scss/_mega-menu.scss

+        }
+      }
+    }
+  }


I can see why we want to do this: We want to hide links in the parent menu, when a submenu is revealed.
But I do have questions:

Is this for display, or for focus trapping, or to control the size of the scrollable area?
(I think the answer is to control height of scrollable area)

Did we not need it before, or did we just forget to add it?

But, I guess it is ok.

there was a bug where submenu level 2 was bigger than first menu, we would have a white page scroll, so that's why was it added

src/themes/default/src/scss/_mega-menu.scss

github-actions bot temporarily deployed to pull request October 30, 2025 13:17 Inactive

donquixote requested changes Nov 3, 2025

View reviewed changes

github-actions bot temporarily deployed to pull request November 3, 2025 21:30 Inactive

tibi2303 force-pushed the OEL-4185-#default# branch from d3d6c2e to bd3e523 Compare November 3, 2025 22:22

github-actions bot temporarily deployed to pull request November 3, 2025 22:27 Inactive

tibi2303 force-pushed the OEL-4185-#default# branch from 13babdc to fb37098 Compare November 3, 2025 22:45

github-actions bot temporarily deployed to pull request November 3, 2025 22:49 Inactive

OEL-4185: Fixed mega menu and update header.

cbc9570

tibi2303 force-pushed the OEL-4185-#default# branch from 032b4f9 to cbc9570 Compare November 17, 2025 13:08

github-actions bot temporarily deployed to pull request November 17, 2025 13:18 Inactive

OEL-4185: See all button fixed.

b699685

github-actions bot temporarily deployed to pull request November 17, 2025 19:49 Inactive

OEL-4185: Fix spacing and improved see all fixed position.

fef56db

github-actions bot temporarily deployed to pull request November 18, 2025 10:08 Inactive

OEL-4185: Improved selectors.

fb2487e

github-actions bot temporarily deployed to pull request November 18, 2025 10:31 Inactive

tibi2303 added 2 commits November 18, 2025 13:22

OEL-4185: Full clickable area.

13b4171

OEL-4185: Prettier.

2146f40

github-actions bot temporarily deployed to pull request November 18, 2025 11:53 Inactive

OEL-4185: Fix span spacing.

7352004

github-actions bot temporarily deployed to pull request November 18, 2025 12:10 Inactive

OEL-4185: Cleanup and markup improvements.

242e6d2

github-actions bot temporarily deployed to pull request November 18, 2025 12:46 Inactive

OEL-4185: Darker border color.

6ad2e10

github-actions bot temporarily deployed to pull request November 18, 2025 17:10 Inactive

donquixote reviewed Nov 18, 2025

View reviewed changes

src/themes/default/src/scss/_mega-menu.scss Outdated Show resolved Hide resolved

donquixote reviewed Nov 18, 2025

View reviewed changes

donquixote added 6 commits November 18, 2025 22:17

OEL-4185: Move the .__see-all style down.

257bbc9

OEL-4185: Move the .see-all-button style into .__see-all section.

8be6bd4

OEL-4185: Remove redundant background color on .__body.

226b2e6

OEL-4185: Rely on flex-basis, instead of block-size: 1px.

af83632

OEL-4185: Drop the .__body wrapper.

8624360

OEL-4185: Update snaps, dropping the .__body wrapper.

4246ded

github-actions bot temporarily deployed to pull request November 18, 2025 22:14 Inactive

donquixote reviewed Nov 18, 2025

View reviewed changes

donquixote approved these changes Nov 19, 2025

View reviewed changes

OEL-4185: Improved mega menu. #702

Are you sure you want to change the base?

OEL-4185: Improved mega menu. #702

Uh oh!

Conversation

tibi2303 commented Oct 30, 2025

Uh oh!

github-actions bot commented Oct 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

donquixote left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tibi2303 Nov 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

donquixote left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

github-actions bot commented Oct 30, 2025 •

edited

Loading

tibi2303 Nov 3, 2025 •

edited

Loading