-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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(css): no emit assets in html style tag (use map) (fix #5968) #6181
Conversation
Co-authored-by: Bjorn Lu <[email protected]>
Co-authored-by: Bjorn Lu <[email protected]>
Co-authored-by: Shinigami <[email protected]>
Co-authored-by: Shinigami <[email protected]>
Co-authored-by: Shinigami <[email protected]>
Co-authored-by: Shinigami <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRC, this PR solves the issue in the build output so that inline styles that are converted into CSS (with a custom class name) are not generated as a separate file, which is denoted by &inline
. This is a wild ride 😄
I've made a few comments below, and we'll probably want to add a test for CSS specificity as well.
I have two concerns though:
&inline
or?inline
is already a valid suffix for CSS imports. I've only discovered this recently and it seems to work like?raw
, though I'm still not familiar how it works. Is it intentional that you choose the&inline
query as well? We might want to rename it in case it clashes somehow.- Probably worth a discussion too if we want to support
url()
instyle=""
? It seems to complicate the flow a lot, though I really appreciate that you've already found a solution for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good to me! I think there's still merit to discuss the below, but it's an approval for me.
Probably worth a discussion too if we want to support
url()
instyle=""
? It seems to complicate the flow a lot
Also if #6048 does have css specificity issue that this PR fixes, we should close that one in favour of this.
yes, we can find the css plugin transfrom it or export transform css method. |
@poyoho I think the line got to long 😱 |
Why is it not automatically formatted🤣 |
I think it has something todo with lint-stage or maybe husky that will come in another PR |
lint-staged is good for me 😀 |
Description
fix #5968
Additional context
I think it is better way to resolve inline style.
generateBundle
.What is the purpose of this pull request?
Before submitting the PR, please make sure you do the following
fixes #123
).