Skip to content
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

feat(cxl-ui): [cxl-marketing-nav] navigation height #265

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

anoblet
Copy link
Collaborator

@anoblet anoblet commented Feb 23, 2023

@anoblet anoblet force-pushed the anoblet/feat/navigation-height branch from 1eb4302 to 5fdd1d6 Compare February 23, 2023 15:55
@github-actions
Copy link

github-actions bot commented Feb 23, 2023

size-limit report 📦

Path Size
packages/cxl-ui/pkg/dist-web/cxl-ui.js 33.46 KB (+0.07% 🔺)
packages/cxl-ui/pkg/dist-web/cxl-ui-jwplayer.js 11.87 KB (0%)
packages/cxl-ui/pkg/dist-web/cxl-ui-playbooks.js 24.9 KB (0%)
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 196.99 KB (+0.02% 🔺)

@lkraav
Copy link

lkraav commented Feb 24, 2023

Code is great, but I can't help to think if it somehow be visually bettered 🤔 what are we missing about mega-menu layout basics - if anything?

image

Weird part is the obvious empty space we create at focus (parent menu item) area.

@heshfekry
Copy link

heshfekry commented Feb 24, 2023

Code is great, but I can't help to think if it somehow be visually bettered 🤔 what are we missing about mega-menu layout basics - if anything?

Udemy has a rather clean menu similar to this.


image

- Our shadow work is a bit over the top. Needs to be toned down in both size and darkness. A smaller lighter shadow.

image

- We need a little separation or at least appear under the main navbar, not overlap

image

- They align text to the left and the arrows to the right eating up some of the margin on that left hand side.

image

@lkraav
Copy link

lkraav commented Feb 24, 2023

This PR should focus only on alignment, and associated white-space.

Udemy example shows some people just live with massive negative space below menu items.

Let's also note that Google apps avoid mega-menus
image

As an SOP, we should probably research well known expert centers like NNGroup and Baymard first. So I'll be looking through https://www.nngroup.com/articles/mega-menus-work-well/ for one, and will post findings later.

@lkraav
Copy link

lkraav commented Feb 24, 2023

One simple action item here could be to improve our limited menu item sample to be more realistic.

These Item #1 titles are too lazy of an example, because

  • we never have that short titles on production sites
  • they generate unnecessary visual layout issues / confusion in QA, phantom problems, delaying progress

@heshfekry
Copy link

heshfekry commented Feb 24, 2023

sample to be more realistic

Indeed our reality makes the below almost entirely redundant.

massive negative space below menu items.

@heshfekry
Copy link

heshfekry commented Feb 24, 2023

I am wondering if aligning the Child menus to top already solves the main issues though. no need to constrain the height of the menu if it introduces unnecessary complexity with scrolling and etc?

image

@lkraav
Copy link

lkraav commented Feb 24, 2023

no need to constrain the height of the menu if it introduces unnecessary complexity with scrolling and etc?

We are not. Our only problematic case is when sub-menu is (significantly) shorter than parent.

But do we even have that live, ever?

And perhaps it signals a problem in itself, that should be fixed at IA level.

@anoblet anoblet force-pushed the anoblet/feat/navigation-height branch from c8c3f2d to b00ce75 Compare February 24, 2023 12:22
@anoblet
Copy link
Collaborator Author

anoblet commented Feb 24, 2023

I added additional navigation items as well as used realistic labels. Moving to the REST API should be a priority for us.

Most mega menus I've encountered have a fixed height(meaning each menu and child menus have the same height). We need to be careful though and add checks so that our logic only runs on wide screens since we haven't tested mobile and the more we customize this, the more likely we will run into issues with responsiveness.

@anoblet anoblet force-pushed the anoblet/feat/navigation-height branch from b00ce75 to 555ed34 Compare February 24, 2023 12:44
@lkraav
Copy link

lkraav commented Feb 24, 2023

Most mega menus I've encountered have a fixed height(meaning each menu and child menus have the same height). We need to be careful though and add checks so that our logic only runs on wide screens since we haven't tested mobile and the more we customize this, the more likely we will run into issues with responsiveness.

Correct, we trigger Vaadin overrides only on wide.

I'm still not convinced that mobile Back button is worth the complicated code it brought in.

@heshfekry
Copy link

heshfekry commented Feb 27, 2023

I'm still not convinced that mobile Back button is worth the complicated code it brought in.

@lkraav If i remember correctly this was a rogue project by syed added unnecessecarily into the spec of sticky nav bar task.

https://app.clickup.com/t/3rgnekt

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants