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

Scope tailwind preflight styles #740

Merged
merged 5 commits into from
Feb 25, 2025

Conversation

Bjoern-Rapp
Copy link
Contributor

@Bjoern-Rapp Bjoern-Rapp commented Jan 29, 2025

Anywidget loads CSS from the _css attribute to the global scope. A widget most take care not to conflict with any other CSS selectors. The stylesheet from the tailwind preflight, loaded with @tailwind base, will conflict with stylesheets from Sphinx themes (#698) or the VScode interactive display (#696).

To scope the preflight stylesheet to descendants of a wrapper should fix that.

@kylebarron
Copy link
Member

Oo interesting; so this would solve #696?

@vgeorge any thoughts?

@Bjoern-Rapp
Copy link
Contributor Author

Yes, that will fix the problem with "leaking" CSS for both #696 and #698.
#696 don't just affects the widget, but the whole notebook/interactive window.

Existing PR
image image

It will not fix the unrelated problem/ feature request of adhering to dark or light themes inside the widget itself.

@Bjoern-Rapp Bjoern-Rapp marked this pull request as ready for review January 30, 2025 14:05
@Bjoern-Rapp Bjoern-Rapp changed the title Attempt too scope tailwind preflight styles Scope tailwind preflight styles Jan 30, 2025
@kylebarron kylebarron requested a review from vgeorge January 30, 2025 15:27
@vgeorge
Copy link
Member

vgeorge commented Jan 30, 2025

@Bjoern-Rapp this is working for me. I tested in vscode and it doesn't seem to be leaking styles anymore. Would you be able to run npm run prettier on your branch and commit the results? This should fix the failing checks.

Comment on lines +2 to +26
color: #11181c;
color: hsl(var(--nextui-foreground));
background-color: #fff;
background-color: hsl(var(--nextui-background));
margin: 0;
line-height: 1.5;
-webkit-text-size-adjust: 100%;
tab-size: 4;
font-family:
ui-sans-serif,
system-ui,
-apple-system,
Segoe UI,
Roboto,
Ubuntu,
Cantarell,
Noto Sans,
sans-serif,
"Apple Color Emoji",
"Segoe UI Emoji",
Segoe UI Symbol,
"Noto Color Emoji";
font-feature-settings: normal;
font-variation-settings: normal;
-webkit-tap-highlight-color: transparent;
Copy link
Member

Choose a reason for hiding this comment

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

@Bjoern-Rapp it didn't notice this when I reviewed yesterday. Could you explain why this was added?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some selectors in the preflight stylesheet targets html, body and :root elements. They become useless when scoped to the div.lonboard wrapper, since there are no html or body elements inside the wrapper. The only alternative then is to copy / duplicate those properties from the resulting stylesheet and apply them directly to the wrapper.

@kylebarron kylebarron requested a review from vgeorge January 31, 2025 16:23
@vgeorge
Copy link
Member

vgeorge commented Feb 20, 2025

@Bjoern-Rapp @kylebarron the background color change appears to be coming from NextUI, not Tailwind. Disabling this line, at least in the VSCode interactive code display, keeps the existing color. Adding overrides to the global styles might work for VSCode, but I would be in favor of a more permanent solution that avoids maintaining the NextUI-related overrides. Currently, we are using only two components from NextUI, Button and Tooltip. I believe we should consider replacing them with custom components and dropping the NextUI dependency as it might conflict with other environments. If you agree we can track this in a separated ticket.

@Bjoern-Rapp
Copy link
Contributor Author

Bjoern-Rapp commented Feb 21, 2025

The change in background colour comes from NextUI, but Tailwind still comes with other changes to the global style that may be unwanted in other environments than VScode, such as Sphinx, if the "base" stylesheet is not scoped in some way. The stylesheet still contains changes to the font family, even when NextUI is removed.
Removing NextUI will reduce the size of the final stylesheet, but isn't really solving the issue.

This solutions should work in all environments, not just VSCode, but there is probably better ways to scope the styles,
or perhaps a way to automatically re-target selectors that targets html, body and :root when scoped?

@kylebarron
Copy link
Member

I'm happy to merge whatever you two agree on as the best approach.

Copy link
Member

@vgeorge vgeorge left a comment

Choose a reason for hiding this comment

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

@Bjoern-Rapp @kylebarron I wasn't able to test on sphinx, but I think we should merge this as it is working well on VSCode and I aggree with Bjørn it should fix on other environments. I still leaning torward on dropping the usage of NextUI a look for a lighter aproach to style components, but let's discuss it on a separated ticket.

@kylebarron kylebarron merged commit 510af74 into developmentseed:main Feb 25, 2025
5 checks passed
@kylebarron
Copy link
Member

Thank you!

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