Skip to content

fix(material/theming): Fix subtle bug in current-selector-or-root #27898

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

Merged
merged 1 commit into from
Oct 5, 2023

Conversation

mmalerba
Copy link
Contributor

@mmalerba mmalerba commented Oct 4, 2023

There was a sublte bug in the previous implementation. Consider the case:

div {
  @include current-selector-or-root() {
    color: red;
  }
  color: green;
}

The previous implementation lifted the color: red; into a separate selector block that came after the initial one, resulting in the order being flipped:

div {
  color: green;
}
div {
  color: red;
}

The new code will produce the correct ordering:

div {
  color: red;
  color: green;
}

@mmalerba mmalerba added the target: major This PR is targeted for the next major release label Oct 4, 2023
@mmalerba mmalerba requested a review from andrewseguin as a code owner October 4, 2023 16:11
Copy link
Contributor

@andrewseguin andrewseguin left a comment

Choose a reason for hiding this comment

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

Nice

Copy link
Member

@crisbeto crisbeto left a comment

Choose a reason for hiding this comment

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

LGTM. I actually had this in my "backlog" to fix since it was bugging me that all CSS variables aren't under the same root 👍

@mmalerba
Copy link
Contributor Author

mmalerba commented Oct 5, 2023

@crisbeto I think if people just @include them at the top-level they'll still be in separate root blocks, but explicitly putting a selector should cause them to be in the same block:

html {
  @include mat.button-theme(...);
  @include mat.checkbox-theme(...);
  ...
}

and most importantly, the order will always remain the same as what the user put

@mmalerba mmalerba added the action: merge The PR is ready for merge by the caretaker label Oct 5, 2023
There was a sublte bug in the previous implementation. Consider the case:

```scss
div {
  @include current-selector-or-root() {
    color: red;
  }
  color: green;
}
```

The previous implementation lifted the `color: red;` into a separate
selector block that came *after* the initial one, resulting in the order
being flipped:

```css
div {
  color: green;
}
div {
  color: red;
}
```

The new code will produce the correct ordering:

```css
div {
  color: red;
  color: green;
}
```
@mmalerba mmalerba requested a review from jelbourn as a code owner October 5, 2023 16:51
@mmalerba mmalerba removed the request for review from jelbourn October 5, 2023 17:10
@mmalerba mmalerba merged commit 999029a into angular:main Oct 5, 2023
zarend pushed a commit that referenced this pull request Oct 6, 2023
…7898)

There was a sublte bug in the previous implementation. Consider the case:

```scss
div {
  @include current-selector-or-root() {
    color: red;
  }
  color: green;
}
```

The previous implementation lifted the `color: red;` into a separate
selector block that came *after* the initial one, resulting in the order
being flipped:

```css
div {
  color: green;
}
div {
  color: red;
}
```

The new code will produce the correct ordering:

```css
div {
  color: red;
  color: green;
}
```
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Nov 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker target: major This PR is targeted for the next major release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants