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

feat: global CSS loader #10036

Merged
merged 2 commits into from
Feb 17, 2021
Merged

feat: global CSS loader #10036

merged 2 commits into from
Feb 17, 2021

Conversation

Haprog
Copy link
Contributor

@Haprog Haprog commented Feb 15, 2021

This makes it so that you can import a CSS file into document scope with an import like:

import './about-view.global.css';

(using a *.global.css filename suffix)
instead of needing something like:

import '!style-loader!css-loader!./about-view.css';

It also makes it so that using a CssResult imported like:

import styles from './my-component.css';

will show a deprecation warning (based on an update to types.d.ts).
To avoid the deprecation and future proof your import you need to use a filename suffix *.shadow.css when you need to import a CSS file as a CssResult like this:

import styles from './my-component.shadow.css';

It's intended that in the next major version (after this has been released) the default behaviour for *.css will change so that you no longer need the .global.css suffix to import styles to document scope but keeping global in the filename will still keep working.

Closes #9970

@Haprog
Copy link
Contributor Author

Haprog commented Feb 15, 2021

If this gets merged, some documentation probably needs to be added to the docs repo about this.

@Haprog
Copy link
Contributor Author

Haprog commented Feb 16, 2021

I just tested this snapshot (<flow.version>7.0.global-css-loader-SNAPSHOT</flow.version>) (using same version for flow-maven-plugin) and it seems to work as intended without any obvious problems except if you have an existing project you need to delete types.d.ts so that it gets updated since we have no upgrade mechanism for it (otherwise you don't get the deprecation behaviour).

@Haprog Haprog marked this pull request as ready for review February 16, 2021 10:53
Copy link
Contributor

@haijian-vaadin haijian-vaadin left a comment

Choose a reason for hiding this comment

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

Just tried, it works as described, great job!
One thing I think could be improved is that when using import styles from './main-view.css';, I only see a warning when trying to use styles, and it doesn't tell what to do.
First, could we show the deprecation warning for the import line instead of when trying to use styles?
Then could it have more information like xxx is deprecated, please use xxx.shadow.css instead?
Screenshot 2021-02-16 at 13 25 21

platosha
platosha previously approved these changes Feb 16, 2021
@Haprog
Copy link
Contributor Author

Haprog commented Feb 16, 2021

First, could we show the deprecation warning for the import line instead of when trying to use styles?

I'm not sure if this is somehow possible.

I can try if adding a note to the deprecation comment in types.d.ts shows it as a deprecation message. That's a good idea if we can show that you should use the shadow suffix instead.

@Haprog
Copy link
Contributor Author

Haprog commented Feb 16, 2021

Added a deprecation message description. See screenshot:
image

It's also shown if you hover over styles on the import line but unfortunately it doesn't highlight the word styles with a strike-through on that line as it does in usages.
image
Seems like that could be fixed later in the IDE itself or some related IDE plugin.

@Haprog
Copy link
Contributor Author

Haprog commented Feb 16, 2021

TeamCity build failed probably due to the scheduled service outage. Let's retry the build a bit later.

@marcushellberg
Copy link
Member

After trying alternatives locally, I ended up preferring query strings to determine how a file is loaded, instead of the filename. That way, the user can determine in each case how they want to load a file.

For example, let's say someone has a small shared.css that they want to load globally. They can do import "./shared.css" or import "./shared.css?global". But they could also load that into a shadow root as a CssResult with import "./shared.css?shadow".

If the filename determines how the file is loaded, the person in the above example would need to create two files with the same content to achieve their use case.

Copy link
Contributor

@haijian-vaadin haijian-vaadin left a comment

Choose a reason for hiding this comment

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

Decided to use filename convention over URL suffixes based on the reasons below:
CSS contents are never independent of their usage, but are rather always bound to some area of the application. The context of usage needs to be defined already when creating a CSS file, at least in the author’s mind — it is essential to know which selectors to use.
When you have a separate CSS file, and the file path does not tell whether it is applied to shadow roots or document styles, it’s a DX issue.
It is even more important for the future maintainer to be able to tell what CSS was for. Consider using a global file content search when maintaining CSS in a large project (often the only tool available). Filename convention makes it easier to confine the search in the certain type of CSS (only shadow or only document).
CSS for both document and shadow roots is an anti-pattern that should be avoided. It never yields efficient and maintainable CSS, unless you really know that you’re building a custom design system.

@haijian-vaadin haijian-vaadin merged commit 70641dd into master Feb 17, 2021
Haprog added a commit that referenced this pull request Feb 17, 2021
* feat: global CSS loader

Closes #9970

* docs: add deprecation message description

(cherry picked from commit 70641dd)
Haprog added a commit that referenced this pull request Feb 17, 2021
* feat: global CSS loader

Closes #9970

* docs: add deprecation message description

(cherry picked from commit 70641dd)
Haprog added a commit that referenced this pull request Feb 19, 2021
manolo pushed a commit that referenced this pull request Feb 19, 2021
Haprog added a commit that referenced this pull request Mar 9, 2021
Haprog added a commit that referenced this pull request Mar 9, 2021
Closes #10105 (since the reverted feature practically caused the need for the issue) and vaadin/docs#193.

* Revert "feat: global CSS loader (#10036)"

This reverts commit 70641dd.

* Revert "fix: add style-loader dependency (#10085)"

This reverts commit f08288c.
@vaadin vaadin deleted a comment from vaadin-bot Apr 26, 2021
@caalador caalador deleted the feature/global-css-loader branch October 12, 2022 10:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change CSS loader configuration
6 participants