feat: update navigation #290

HenerHoop · 2023-07-05T19:12:33Z

Could not find a way to change the box shadow on vaadin-context-menu-list-box elements

https://app.clickup.com/t/861mx0nwm

https://www.figma.com/file/RTj2PtUSrCOUFtljOiq0W3/Dashboard?type=design&node-id=291-78069&mode=design

github-actions · 2023-07-05T19:14:26Z

size-limit report 📦

Path	Size
packages/cxl-ui/pkg/dist-web/cxl-ui.js	34.03 KB (+0.27% 🔺)
packages/cxl-ui/pkg/dist-web/cxl-ui-jwplayer.js	11.87 KB (0%)
packages/cxl-ui/pkg/dist-web/cxl-ui-playbooks.js	25.39 KB (+0.41% 🔺)
packages/cxl-ui/pkg/dist-web/vendor.js	125.6 KB (0%)
packages/cxl-ui/pkg/dist-web/cxl-ui-jwplayer.js, packages/cxl-ui/pkg/dist-web/cxl-ui-playbooks.js, packages/cxl-ui/pkg/dist-web/cxl-ui.js, packages/cxl-ui/pkg/dist-web/manifest.js, packages/cxl-ui/pkg/dist-web/unresolved.js, packages/cxl-ui/pkg/dist-web/vendor.js	198.04 KB (+0.1% 🔺)

pawelkmpt

Looking good so far but some work still to be done. Keep going :)

Also take into account such situation:

packages/cxl-lumo-styles/scss/themes/vaadin-context-menu-item.scss

packages/cxl-lumo-styles/scss/themes/vaadin-context-menu-list-box.scss

packages/cxl-ui/src/components/cxl-marketing-nav.js

pawelkmpt · 2023-07-07T10:00:19Z

@HenerHoop can I do in-deep review comparing to the design or you'll push some changes soon?

HenerHoop · 2023-07-07T10:10:03Z

@pawelkmpt I'll push some changes soon.

packages/cxl-ui/src/components/cxl-marketing-nav.js

pawelkmpt

@HenerHoop menu disappears during navigation. Take a look: https://www.loom.com/share/b58641f372be428189780c00bd227894?sid=a9793a0c-b72c-4568-bffb-0196c0120306

packages/cxl-lumo-styles/scss/themes/vaadin-overlay.scss

HenerHoop · 2023-07-10T10:24:59Z

@pawelkmpt I accidentally pushed a few extra lines as well. You can now take another look.

pawelkmpt · 2023-07-10T13:25:29Z

@pawelkmpt I accidentally pushed a few extra lines as well. You can now take another look.

it's better now 👍

pawelkmpt

Paddings (left/right) are too big. I didn't measure them but at the first glance they're bigger than in the design.

Scroll for above pictured item doesn't work.
Black bar items on the left should be gray

pawelkmpt · 2023-07-10T13:28:30Z

Could not find a way to change the box shadow on vaadin-context-menu-list-box elements

It is working nice now, good job

pawelkmpt · 2023-08-14T12:22:40Z

On mobile hovering on the item with descendants opens submenu. It doesn't work like that in other places. Need fixes

freudFlintstone · 2023-08-17T02:09:57Z

packages/cxl-ui/src/components/cxl-marketing-nav.js

+  /*
+  * Setup listeners on nav items to handle overlays events.
+  */
+  _setupSlottedMenuItems () {


This fixes the bug discussed here. Credit to @anoblet for finding the solution.

freudFlintstone · 2023-08-17T02:11:33Z

packages/cxl-ui/scss/cxl-marketing-nav.scss

+  // Second shadow set to emulate border
+  box-shadow: 0 8px 24px 0 rgba(0, 0, 0, 0.25), 0 0 1px 1px #D2D5DA;
+  opacity: 1;
+  transition: opacity 0.2s, left 0.1s;


animations are not meant for aesthetics. They help hide an ugly flickering that happens because the menu is initally appended to the body and needs to be moved to the overlays-wrapper.

pawelkmpt

@freudFlintstone I tested it on WPS and z-index issue occurs

pawelkmpt · 2023-08-24T09:16:10Z

packages/cxl-ui/src/components/cxl-marketing-nav.js

@@ -318,7 +362,11 @@ export class CXLMarketingNavElement extends LitElement {

        menuItem.appendChild(link);

+        // If item is at depth 1 and had no children, assume it's a section header, do not show description
+        if (item.sectionHeader && !item.children) {


@freudFlintstone I need this to be lowercase item.sectionheader because tree JSON serializer we use in the PHP layer turns all keys into lowercase, I can't force it to camelCase and therefore section headers do not have background.

Also it should not be limited to a component. I think section headers won't be of type component => a.

About the component, I'm not 100% sure why it's implemented like that, but I tested a couple ideas and the it needs that to render the items correctly. I can look into it a little deeper tomorrow.
But the <a> tag is created by cxl-marketing-nav and used in the menu list as a wrapper, so you are not obligated to pass only <a> components from your side. As long as it's valid HTML or plain text, it will be properly rendered.

About the component, I'm not 100% sure why it's implemented like that, but I tested a couple ideas and the it needs that to render the items correctly. I can look into it a little deeper tomorrow.

I do not agree. I just experimented and adding following code before if (item.component === 'a') will do what I want:

add section header item,

no link, just item.

if (item.sectionheader) { const menuItem = document.createElement('vaadin-context-menu-item'); menuItem.classList.add('section-header'); }

Is there anything wrong with adding context menu item without <a> inside @freudFlintstone @anoblet ?

But the tag is created by cxl-marketing-nav and used in the menu list as a wrapper, so you are not obligated to pass only components from your side. As long as it's valid HTML or plain text, it will be properly rendered.

At PHP level there's a condition which defines the component. Later in JS actual HTML element is created but as you can notice - there are conditions like:

if (item.component === 'a')

if (item.component === 'back').

Maybe instead of working with sectionheader property we should define new component type?

I'm fine with your solution for sectionheader, the difference in styling is too minimal to justify a different component just for that.
For the <a> tag, no, it seems nothing that makes that mandatory, it was just a choice made years ago. It seems to be there just for creating links for items that have href, but it's use as the default for other items too, unless they are set to hr or back.
Again, if checking for sectionheader before works, great, but we are probably still gonna need the <a> when href is set.

@pawelkmpt rolling back these changes, since Hesh told us we will need to support links in the future

There's still a small issue with that section label, because the figma spec uses a font-weight of 400, which we don't include in the current font. It is using a bolder 600 for now.

packages/storybook/cxl-ui/cxl-marketing-nav.data.json

https://app.clickup.com/t/861mx0nwm

…font-weight

pawelkmpt

Gradients mess up mobile view for menu

pawelkmpt · 2023-08-29T08:34:54Z

@freudFlintstone let me show on examples what I mean by possibility to have menu section header WITHOUT a link/being a component.

CASE 1

Following params passed. URL is empty string.

$menu_item['component']     = 'a';
$menu_item['description']   = $item->description;
$menu_item['href']          = $item->url;
$menu_item['sectionheader'] = true;

Result: Deep skills links to the current URL -> ❌ wrong

CASE 2

Following params passed. No href item.

$menu_item['component']     = 'a';
$menu_item['description']   = $item->description;
$menu_item['sectionheader'] = true;

Result: Deep skills link is undefined -> ❌ wrong

CASE 3

Following params passed. It's section header and component not defined.

$menu_item['description']   = $item->description;
$menu_item['sectionheader'] = true;

Result: Deep skills has not background and no description -> ❌ wrong

What I expect from section header item:

background,
description,
link only when href is not empty.

Please provide such tweak and tell me what params I shall pass to JS in order to make it work.

pawelkmpt · 2023-08-29T08:38:43Z

Bug

Context menu items are positioned differently for logged in and guest users.
Possible reason: logged in users see admin bar at the top and JS doesn't take it into account.

✅ CORRECT for guests

❌ INCORRECT for logged in users

…l positioning when logged in to wp

freudFlintstone

Thank you for the detailed review @pawelkmpt , it really helped. This commit fixes it, but does not allow links to be used for section-header, for now. It does not have a hover state either, so users don't mistake it for a link.
The vertical positioning now works when logged to WP. There's a toggle in the component's story to test it.

HenerHoop requested review from lkraav, pawelkmpt and anoblet July 5, 2023 20:47

pawelkmpt requested changes Jul 6, 2023

View reviewed changes

pawelkmpt requested a review from heshfekry July 6, 2023 07:38

pawelkmpt changed the base branch from master to feat/navigation July 6, 2023 08:06

HenerHoop force-pushed the hener/feat/navigation-update branch from c842a00 to 0418937 Compare July 6, 2023 15:46

pawelkmpt requested changes Jul 7, 2023

View reviewed changes

packages/cxl-ui/src/components/cxl-marketing-nav.js Outdated Show resolved Hide resolved

HenerHoop force-pushed the hener/feat/navigation-update branch from 0418937 to 3ab4e04 Compare July 7, 2023 14:11

HenerHoop requested a review from pawelkmpt July 7, 2023 14:12

HenerHoop force-pushed the hener/feat/navigation-update branch from 3ab4e04 to 5463dca Compare July 8, 2023 15:09

anoblet requested changes Jul 8, 2023

View reviewed changes

packages/cxl-ui/src/components/cxl-marketing-nav.js Outdated Show resolved Hide resolved

HenerHoop force-pushed the hener/feat/navigation-update branch from 5463dca to 1118e4e Compare July 9, 2023 12:50

pawelkmpt assigned pawelkmpt and unassigned pawelkmpt Jul 10, 2023

pawelkmpt requested changes Jul 10, 2023

View reviewed changes

packages/cxl-lumo-styles/scss/themes/vaadin-overlay.scss Show resolved Hide resolved

HenerHoop force-pushed the hener/feat/navigation-update branch from 1118e4e to 8777591 Compare July 10, 2023 10:20

pawelkmpt requested changes Jul 10, 2023

View reviewed changes

HenerHoop force-pushed the hener/feat/navigation-update branch from 8777591 to ca656b3 Compare July 11, 2023 06:08

HenerHoop requested a review from pawelkmpt July 11, 2023 10:20

pawelkmpt added the CXL 2.0 label Jul 26, 2023

freudFlintstone force-pushed the hener/feat/navigation-update branch 2 times, most recently from 3511a3a to 1895ada Compare August 17, 2023 02:05

freudFlintstone reviewed Aug 17, 2023

View reviewed changes

freudFlintstone requested a review from anoblet August 17, 2023 02:12

pawelkmpt requested changes Aug 18, 2023

View reviewed changes

freudFlintstone force-pushed the hener/feat/navigation-update branch 2 times, most recently from 2a60426 to bede9e7 Compare August 18, 2023 21:13

freudFlintstone requested a review from pawelkmpt August 18, 2023 21:14

freudFlintstone force-pushed the hener/feat/navigation-update branch 2 times, most recently from 0c4031d to 279ebfe Compare August 18, 2023 22:59

pawelkmpt requested changes Aug 24, 2023

View reviewed changes

pawelkmpt reviewed Aug 24, 2023

View reviewed changes

packages/storybook/cxl-ui/cxl-marketing-nav.data.json Outdated Show resolved Hide resolved

Hener Hoop and others added 4 commits August 24, 2023 23:48

feat: update navigation

ad561f1

https://app.clickup.com/t/861mx0nwm

feat(cxl-ui): implement new navigation design

7477877

feat(cxl-ui): place overlays in body, to avoid stacking context issues

ca8a12f

fix(cxl-ui): expect lowercase fields in nav items data, menu headers …

41b7ab1

…font-weight

freudFlintstone force-pushed the hener/feat/navigation-update branch from 279ebfe to 41b7ab1 Compare August 25, 2023 03:30

freudFlintstone requested a review from pawelkmpt August 25, 2023 03:30

pawelkmpt requested changes Aug 25, 2023

View reviewed changes

fix(cxl-marketing-nav): Include descriptions in menu section headers

274d527

freudFlintstone force-pushed the hener/feat/navigation-update branch from 46819eb to 274d527 Compare August 28, 2023 14:44

fix(cxl-marketing-nav): fix regression in mobile layout menu

3bf4898

fix(cxl-marketing-nav): do not use <a> for sectionheader, fix vertica…

b311beb

…l positioning when logged in to wp

freudFlintstone force-pushed the hener/feat/navigation-update branch from 7c57ef9 to b311beb Compare August 29, 2023 23:42

freudFlintstone requested a review from pawelkmpt August 29, 2023 23:43

freudFlintstone reviewed Aug 29, 2023

View reviewed changes

pawelkmpt approved these changes Aug 30, 2023

View reviewed changes

pawelkmpt merged commit 2302393 into feat/navigation Aug 30, 2023
4 checks passed

pawelkmpt deleted the hener/feat/navigation-update branch January 9, 2024 12:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: update navigation #290

feat: update navigation #290

HenerHoop commented Jul 5, 2023 •

edited by pawelkmpt

Loading

github-actions bot commented Jul 5, 2023

pawelkmpt left a comment

pawelkmpt commented Jul 7, 2023

HenerHoop commented Jul 7, 2023

pawelkmpt left a comment

HenerHoop commented Jul 10, 2023

pawelkmpt commented Jul 10, 2023

pawelkmpt left a comment

pawelkmpt commented Jul 10, 2023

pawelkmpt commented Aug 14, 2023

freudFlintstone Aug 17, 2023

freudFlintstone Aug 17, 2023

pawelkmpt left a comment

pawelkmpt Aug 24, 2023

pawelkmpt Aug 24, 2023

freudFlintstone Aug 25, 2023 •

edited

Loading

pawelkmpt Aug 25, 2023

pawelkmpt Aug 25, 2023

freudFlintstone Aug 25, 2023

freudFlintstone Aug 28, 2023

freudFlintstone Aug 28, 2023

pawelkmpt left a comment

pawelkmpt commented Aug 29, 2023 •

edited

Loading

pawelkmpt commented Aug 29, 2023

freudFlintstone left a comment

feat: update navigation #290

feat: update navigation #290

Conversation

HenerHoop commented Jul 5, 2023 • edited by pawelkmpt Loading

github-actions bot commented Jul 5, 2023

size-limit report 📦

pawelkmpt left a comment

Choose a reason for hiding this comment

pawelkmpt commented Jul 7, 2023

HenerHoop commented Jul 7, 2023

pawelkmpt left a comment

Choose a reason for hiding this comment

HenerHoop commented Jul 10, 2023

pawelkmpt commented Jul 10, 2023

pawelkmpt left a comment

Choose a reason for hiding this comment

pawelkmpt commented Jul 10, 2023

pawelkmpt commented Aug 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pawelkmpt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

freudFlintstone Aug 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pawelkmpt left a comment

Choose a reason for hiding this comment

pawelkmpt commented Aug 29, 2023 • edited Loading

CASE 1

CASE 2

CASE 3

pawelkmpt commented Aug 29, 2023

Bug

freudFlintstone left a comment

Choose a reason for hiding this comment

HenerHoop commented Jul 5, 2023 •

edited by pawelkmpt

Loading

freudFlintstone Aug 25, 2023 •

edited

Loading

pawelkmpt commented Aug 29, 2023 •

edited

Loading