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

upgrade vaadin #31

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

upgrade vaadin #31

wants to merge 5 commits into from

Conversation

anoblet
Copy link
Owner

@anoblet anoblet commented Dec 1, 2021

No description provided.

@github-actions
Copy link

github-actions bot commented Dec 1, 2021

size-limit report 📦

Path Size
packages/cxl-ui/pkg/dist-web/index.bundled.js 108.13 KB (+6.88% 🔺)

@lkraav
Copy link
Collaborator

lkraav commented Dec 1, 2021

size-limit report package

Path Size
packages/cxl-ui/pkg/dist-web/index.bundled.js 112.5 KB (+11.2% small_red_triangle)

Wow, +10%? Unlucky.

@anoblet
Copy link
Owner Author

anoblet commented Dec 1, 2021

There's some issues with:

Error: An item in styles is not of type CSSResult. Use `unsafeCSS` or `css`.

I'm going to debug it today.

@anoblet
Copy link
Owner Author

anoblet commented Dec 1, 2021

Wow, +10%? Unlucky.

It will be nice when Vaadin uses Lit instead of Polymer :)

@lkraav
Copy link
Collaborator

lkraav commented Dec 1, 2021

There's some issues with:

Error: An item in styles is not of type CSSResult. Use `unsafeCSS` or `css`.

Depends on where.

Maybe https://github.com/conversionxl/aybolit/blob/master/packages/cxl-lumo-styles/src/utils.js needs to be updated with their latest.

@anoblet
Copy link
Owner Author

anoblet commented Dec 1, 2021

My first thought is that when the SCSS is parsed, it's being passed in as a string instead of a CSSResult. If we could wrap that in a unsafeCSS directive that might work.

I don't see any significant differences between our version of utils.js and master (https://github.com/vaadin/vaadin-themable-mixin/commits/master/register-styles.html).

@anoblet
Copy link
Owner Author

anoblet commented Dec 2, 2021

Wrapping the input with css/unsafeCSS seems to work. Now I'm running into:

Could not find style data in module named lumo-color
Could not find style data in module named lumo-typography

These posts make it seem environment related:

@anoblet
Copy link
Owner Author

anoblet commented Dec 2, 2021

Removing include="lumo-color" and include="lumo-typography" from:

get's rid of the errors though the typography still doesn't render correctly.

@lkraav
Copy link
Collaborator

lkraav commented Dec 3, 2021

With https://mobile.twitter.com/serhiikulykov/status/1466137314082660356 perhaps also look into going for optimized imports like @vaadin/context-menu.

@anoblet
Copy link
Owner Author

anoblet commented Dec 6, 2021

Typography is working now.

@lkraav
Copy link
Collaborator

lkraav commented Dec 7, 2021

Looking at https://github.com/vaadin/web-components/blob/e2b36f260eae94ac249fc9f7f1dff43e6228e52a/packages/polymer-legacy-adapter/src/style-modules.js#L8, can we also upgrade to Lit in this PR?

For some reason, cxl-app-layout media query stuff is still in here, too.

@anoblet
Copy link
Owner Author

anoblet commented Dec 7, 2021

Vaadin dropped support for vaadin-device-detector and it isn't available for import from @vaadin/vaadin-context-menu anymore. The observe function replaces it, though it may be better to put it into a utility file rather than at the top of cxl-app-layout and import it.

I'll start working on the import refactoring and lit upgrade.

@lkraav
Copy link
Collaborator

lkraav commented Dec 7, 2021

Vaadin dropped support for vaadin-device-detector and it isn't available for import from @vaadin/vaadin-context-menu anymore.

Hm, but https://github.com/vaadin/web-components/blob/master/packages/context-menu/src/vaadin-device-detector.js is still there today?

@anoblet
Copy link
Owner Author

anoblet commented Dec 7, 2021

It's there, I'm not sure why it won't import.

image

@lkraav
Copy link
Collaborator

lkraav commented Dec 7, 2021

It's there, I'm not sure why it won't import.

image

Because @vaadin/context-menu, not @vaadin/vaadin-....

@anoblet
Copy link
Owner Author

anoblet commented Dec 7, 2021

I see :)

@anoblet
Copy link
Owner Author

anoblet commented Dec 7, 2021

I'll revert the commit and fix it in the import refactor.

@anoblet
Copy link
Owner Author

anoblet commented Dec 7, 2021

@vaadin/vaadin-lumo-styles and @vaadin/vaadin-themable-mixin don't follow the new naming convention.

@anoblet
Copy link
Owner Author

anoblet commented Dec 7, 2021

@lkraav Let me know your thoughts and I will PR upstream.

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.

2 participants