conversionxl / aybolit

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

fix(cxl-ui): fix app-layout wide media query #350

Closed freudFlintstone closed 11 months ago

freudFlintstone commented 11 months ago

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

github-actions[bot] commented 11 months ago

size-limit report 📦

Path Size
packages/cxl-ui/pkg/dist-web/cxl-ui.js 63.65 KB (+0.01% 🔺)
packages/cxl-ui/pkg/dist-web/cxl-ui-jwplayer.js 11.87 KB (0%)
packages/cxl-ui/pkg/dist-web/cxl-ui-playbooks.js 27.66 KB (0%)
packages/cxl-ui/pkg/dist-web/vendor.js 135.58 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 239.91 KB (+0.01% 🔺)
lkraav commented 11 months ago

I wonder if we should take the opportunity to try out @lit/context here for wide state, instead of trying to piece together this media query puzzle across components? @anoblet your thoughts?

freudFlintstone commented 11 months ago

I wonder if we should take the opportunity to try out @lit/context here for wide state, instead of trying to piece together this media query puzzle across components? @anoblet your thoughts?

If we were fully lit based, I would say a ReactiveController using ResizeObserver is more effective. But Context would allow us to do it right now.

Also, as I mentioned here, I think this requires a proper task to produce an app wide fix. Do you think we could deploy this, as it's very broken in prod now, and work on a Context based solution in the next sprint?

lkraav commented 11 months ago

Do you think we could deploy this, as it's very broken in prod now, and work on a Context based solution in the next sprint?

Yes, but add @todo annotations in these cases to at least somewhat help avoid temporary things becoming forever.

anoblet commented 11 months ago

I'm still trying to understand the reasoning behind the Context API. It feels like state management baked into the browser. In this case I would recommend https://github.com/adobe/lit-mobx. Almost zero boilerplate.

There wouldn't be much of a performance boost, though we could keep track of media changes across components without having to recalculate, and update accordingly.

freudFlintstone commented 11 months ago

@anoblet You're right about that, enve the Lit team themselves recommend mobX or Redux if we are doing global, shared state management. But Context is supposed to be lightweight and easy to use if we are doing things like sharing user info, or mostly static properties. Device/viewport media queries basically never change, unless on mobile device rotation, so I'd still try Context first.

anoblet commented 11 months ago

@freudFlintstone If you could create a POC showing the upside of using context, that would be helpful. I'm not sure of the performance gains other than technique. Let me know how I can help 👍

lkraav commented 11 months ago

I'm not sure of the performance gains other than technique. Let me know how I can help 👍

We're not looking for performance gains. More DX sanity is the goal. After vaadin-device-detector as the central wide state controller went away, things started to get unglued. We have multiple components interested, and a serious layout bug that snuck live, that's enough signal to me. Seems like worth 1-2h effort simple context system integration should take.