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): re-implement navigation bar #341

Merged
merged 18 commits into from
Dec 15, 2023

Conversation

freudFlintstone
Copy link

https://app.clickup.com/t/86ayah2zr

POC for navigation component using vaadin-menu-bar instead of vaadin-tabs, aiming to have a standard context menu UI.
Because the menu-bar component also uses vaadin-context-menu under the hood, it looks and behaves largely the same as the current custom implementation, even with little CSS work.
Initial implementation brings the navigation component down from nearly 700 lines of code to about 80. If this is used in place of the current component, a lot of CSS can also be cleaned up.

@github-actions
Copy link

github-actions bot commented Oct 2, 2023

size-limit report 📦

Path Size
packages/cxl-ui/pkg/dist-web/cxl-ui.js 69.53 KB (+1.07% 🔺)
packages/cxl-ui/pkg/dist-web/cxl-ui-jwplayer.js 11.89 KB (-0.01% 🔽)
packages/cxl-ui/pkg/dist-web/cxl-ui-playbooks.js 28.79 KB (+2.26% 🔺)
packages/cxl-ui/pkg/dist-web/vendor.js 140.5 KB (+3.26% 🔺)
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 251.87 KB (+2.36% 🔺)

@freudFlintstone freudFlintstone force-pushed the raphael/feat/navigation/update-component branch 2 times, most recently from f621126 to b35e2de Compare October 11, 2023 02:23
@freudFlintstone freudFlintstone force-pushed the raphael/feat/navigation/update-component branch from b35e2de to 2f640f8 Compare October 11, 2023 02:46
@pawelkmpt pawelkmpt requested a review from lkraav October 11, 2023 08:22
pawelkmpt

This comment was marked as resolved.

@pawelkmpt
Copy link

pawelkmpt commented Oct 11, 2023

WPS

For given template changes:

Screenshot 2023-10-11 at 10 53 22

This is the result. On the left old menu, right new.

Screenshot 2023-10-11 at 10 51 29
Screenshot 2023-10-11 at 10 51 49

Screenshot 2023-10-11 at 10 54 26

Screenshot 2023-10-11 at 10 54 38

@freudFlintstone
Copy link
Author

I'll update my branch to get rid of the old component and avoid WPS changes. I have 110 places to check in aybolit, so it will take me a little while.
@pawelkmpt Also, what are those extra menu items? Is there a way I should be looking to filter them out? There doesn't seem to be like that in the old component.

@pawelkmpt
Copy link

@pawelkmpt Also, what are those extra menu items? Is there a way I should be looking to filter them out? There doesn't seem to be like that in the old component.

"After categories" came from other menu tree.
Top arrows -> IDK

@freudFlintstone freudFlintstone force-pushed the raphael/feat/navigation/update-component branch 2 times, most recently from cfb17a9 to 7e8735d Compare October 12, 2023 22:08
@freudFlintstone freudFlintstone force-pushed the raphael/feat/navigation/update-component branch from 9c220ca to debe3d7 Compare October 13, 2023 02:57
Copy link
Author

@freudFlintstone freudFlintstone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pawelkmpt The old component is now completely gone, including any styles in cxl-ui and cxl-lumo-styles that were realted to it. The odd menu items, however, need more investigating, and possibily some database cleanup. It's also likely there are WPS styles overriding the color of the text on the global bar.

@freudFlintstone freudFlintstone self-assigned this Oct 13, 2023
@freudFlintstone freudFlintstone marked this pull request as ready for review October 13, 2023 11:28
@freudFlintstone freudFlintstone force-pushed the raphael/feat/navigation/update-component branch from debe3d7 to 0a341ca Compare October 16, 2023 14:19
packages/cxl-ui/src/components/cxl-marketing-nav.js Outdated Show resolved Hide resolved
packages/cxl-ui/src/components/cxl-marketing-nav.js Outdated Show resolved Hide resolved
packages/cxl-ui/src/components/cxl-marketing-nav.js Outdated Show resolved Hide resolved
packages/cxl-ui/src/components/cxl-marketing-nav.js Outdated Show resolved Hide resolved
@freudFlintstone freudFlintstone marked this pull request as draft October 18, 2023 02:17
@freudFlintstone freudFlintstone force-pushed the raphael/feat/navigation/update-component branch from 0f6cce9 to 5cbe29e Compare October 18, 2023 02:23
@freudFlintstone freudFlintstone force-pushed the raphael/feat/navigation/update-component branch 4 times, most recently from 5737311 to 711d7db Compare October 19, 2023 23:42
@freudFlintstone freudFlintstone force-pushed the raphael/feat/navigation/update-component branch from 2e33907 to b2415bb Compare November 30, 2023 13:48
@freudFlintstone
Copy link
Author

@freudFlintstone freudFlintstone force-pushed the raphael/feat/navigation/update-component branch from b2415bb to 0d8b3f8 Compare December 8, 2023 12:03
@pawelkmpt
Copy link

This got messy. I locally did git rebase --ff master and don't have recent master commits mixed with this branch

Screenshot 2023-12-14 at 16 09 48

@freudFlintstone
Copy link
Author

Hum, something definitely odd happened here:
image

@pawelkmpt pawelkmpt force-pushed the raphael/feat/navigation/update-component branch from 36b2f3d to d30a334 Compare December 15, 2023 09:00
@pawelkmpt
Copy link

Hum, something definitely odd happened here: image

It was still a mess. I cleaned, rebased and force pushed @freudFlintstone

@pawelkmpt pawelkmpt force-pushed the raphael/feat/navigation/update-component branch from d30a334 to c1784bb Compare December 15, 2023 09:17
@pawelkmpt pawelkmpt merged commit 72e94f0 into master Dec 15, 2023
4 of 5 checks passed
@pawelkmpt pawelkmpt deleted the raphael/feat/navigation/update-component branch December 15, 2023 09:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants