search

conversionxl / aybolit

Lightweight web components library built with LitElement.
https://conversionxl.github.io/aybolit/
MIT License
7 stars 8 forks source link

feat(cxl-ui): cxl-marketing-nav tweaks #238

Closed saas786 closed 1 year ago

saas786 commented 1 year ago

Tweaks for: https://app.clickup.com/t/2z64umf

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

Dep: https://github.com/conversionxl/wp-theme-lib/pull/66

github-actions[bot] commented 1 year ago

size-limit report 📦

Path Size
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 65.7 KB (+4.74% 🔺)
saas786 commented 1 year ago

LGTM. I agree with @lkraav though that there is a fair amount of complexity in this file that could be refactored. I'm not sure if controllers, mixins, or pure functions could be useful here?

One for the future, we can consider breaking cxl-marketing-nav into logical parts.

saas786 commented 1 year ago

cxl-marketing-nav.js is getting out of control.

How can we separate back button implementation into an importable file?

(I also have a refactoring done for configuring search box setup, which will reduce firstUpdated() method size and complexity. Maybe this should also go into a separate file.)

Certainly, and such thought popups in my mind too, but didn't get to it so far, also this PR is not intended for such task.

Can tackle in this cu-task: https://app.clickup.com/t/3vccuxg

kylebrodeur commented 1 year ago

Task linked: CU-3rgnekt cxl-marketing-nav mobile there is no back arrow to go one level up