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: global with pseudo element should be considered as global #6317

Merged

Conversation

roychoo
Copy link
Contributor

@roychoo roychoo commented May 9, 2021

Hi,
in PR, https://github.com/sveltejs/svelte/pull/6223/files#diff-145a1fa56c966be609d17f4af6eb9c2a0a97cd2f1c9c86bf64841595f254f51dR597, an additional check was introduced to make sure the number of selectors is equal to 1, however this also covers pseudo elements which in turn break some of the existing capabilities as describe in #6306

 .container :global(section) > :global(*):first-child {
	color: red;
}

global(*):first-child is no longer considered as a global where previously it was considered.

Thus, adding a negation check that if there is a ClassSelector, it should be excluded from global check.

cc: @tanhauhau (PR #6223 author)

@alcore
Copy link

alcore commented May 14, 2021

We've had to pin to 3.37 for now, due to the issue this PR attempts to solve, as #6223 broke syntax that was previously valid using preprocessors' nesting. Before 3.38 this compiled fine (SASS):

div
  :global(p)
    &:after, &:before
      color: red

Where div had the component's hashed class applied and any :global child had its styles applied correctly too - this is not currently the case, and they are not applied at all, silently.

I.e. unlike with nesting ... :global(...) :global(...) which throws an error (where it was legal previously), pseudoclasses get dropped and result in a silently broken stylesheet compilation for anyone utilizing this pattern.

@7antra
Copy link

7antra commented May 21, 2021

It will be interesting to merge this PR soon, because now in the 1.0.0-next.106 of @svelte/kit the minimum version accepted for svelte (peerDependencies) is 3.38.2

@roychoo
Copy link
Contributor Author

roychoo commented May 21, 2021

Hi, since it had been some time, is there any feedback on this proposed fix in this PR?

@7antra
Copy link

7antra commented Jun 3, 2021

@roychoo Do you know how to speed up the merging process of this PR? Because for all developers who use svelte + scss this ":global()" bug is very annoying and prevents the update :'(

@roychoo
Copy link
Contributor Author

roychoo commented Jun 3, 2021

@roychoo Do you know how to speed up the merging process of this PR? Because for all developers who use svelte + scss this ":global()" bug is very annoying and prevents the update :'(

I am not sure myself, this is my first time contributing to svelte

@non25
Copy link

non25 commented Jun 10, 2021

Shameless plug: sveltejs/rfcs#13 (comment)

@Conduitry Conduitry merged commit 228832c into sveltejs:master Jun 22, 2021
@roychoo
Copy link
Contributor Author

roychoo commented Jun 22, 2021

Thanks for the adjustment and merged

@7antra
Copy link

7antra commented Jun 22, 2021

Thank you @Conduitry <3

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.

5 participants