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: import global theme to enable Lumo utility classes #3606

Closed
wants to merge 3 commits into from

Conversation

web-padawan
Copy link
Member

This fixes the ContextMenu styling example which is currently broken due to the fact that Lumo utility classes are no longer imported globally.

As far as I understand, this is a result of the Flow change which moved global Lumo imports including utility-global.js to the theme-docs.global.generated.js and that file isn't currently imported (since applyTheme helper used in examples is exported from theme-docs.generated.js which no longer contains global modules).

Copy link
Member

@tomivirkki tomivirkki Aug 21, 2024

Choose a reason for hiding this comment

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

I wonder if this should be fixed in the webpack.dspublisher.js instead so it would also affect custom themes. Perhaps this line needs an update?

Copy link
Member Author

Choose a reason for hiding this comment

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

How would you suggest to change it?

BTW, just noticed that generated-flow-imports.js includes color-global.js and typography-global.js but not utility-global.js. I guess it's because of the fact that first two are included using @JsModule annotation on the Lumo class, while badge-global and utility-global are not.

Copy link
Member

@tomivirkki tomivirkki Aug 21, 2024

Choose a reason for hiding this comment

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

Right, I see. It would probably be appropriate to add import '@vaadin/vaadin-lumo-styles/utility-global.js'; to init-browser.ts. This way we'll not have a direct import to a theme name-specific generated file.

Copy link
Member Author

Choose a reason for hiding this comment

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

Tested it and it works for docs pages, but not when opening example in a new window.

I tried to add import '../generated/vaadin' but that fails with the following error:

Failed to compile
../../../../../../vaadin/docs/frontend/generated/index.tsx

Module not found: Can't resolve 'Frontend/routes.js' in '/Users/serhii/vaadin/docs/frontend/generated'

Copy link
Member

Choose a reason for hiding this comment

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

Tested it and it works for docs pages, but not when opening example in a new window

import '@vaadin/vaadin-lumo-styles/utility-global.js'; in this file seems to work for both cases but it's not ideal either.

The original fix should be good enough since we can assume "docs" theme to be available in projects using a custom theme also, assuming they set it as the parent theme for "docs" as documented...otherwise, some other examples would be missing essential styles also.

@web-padawan
Copy link
Member Author

Looks like I included this change by accident to #3587. Closing.

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.

3 participants