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

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

Merged
merged 1 commit into from
Oct 26, 2023

Conversation

freudFlintstone
Copy link

@github-actions
Copy link

github-actions bot commented Oct 25, 2023

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
Copy link

lkraav commented Oct 25, 2023

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
Copy link
Author

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
Copy link

lkraav commented Oct 25, 2023

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.

@freudFlintstone freudFlintstone force-pushed the raphael/fix/blog/broken-mobile-layout branch from 20913cd to cf50b1a Compare October 25, 2023 21:42
@pawelkmpt pawelkmpt merged commit 34af6a3 into master Oct 26, 2023
4 checks passed
@pawelkmpt pawelkmpt deleted the raphael/fix/blog/broken-mobile-layout branch October 26, 2023 09:45
@anoblet
Copy link
Collaborator

anoblet commented Oct 26, 2023

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
Copy link
Author

@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
Copy link
Collaborator

anoblet commented Oct 26, 2023

@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
Copy link

lkraav commented Oct 26, 2023

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.

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.

4 participants