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(material/snack-bar): Switch snack-bar implementation to use MDC #25458

Merged
merged 1 commit into from Aug 16, 2022

Conversation

mmalerba
Copy link
Contributor

Old implementation is still available under @angular/material/legacy-snack-bar

BREAKING CHANGE:

  • DOM and CSS classes for mat-snack-bar have changes
  • Typescript API is largely the same but may have minor differences
  • See the MDC migration guide for more information about the changes and
    how to migrate your app (TODO: link when available)

@mmalerba mmalerba added the target: major This PR is targeted for the next major release label Aug 12, 2022
@mmalerba mmalerba force-pushed the mdc-snack-bar branch 2 times, most recently from 6d8a7f7 to 3e70e0b Compare August 12, 2022 20:40
@mmalerba
Copy link
Contributor Author

@crisbeto do you know whats up with this test failure: "theming api should warn if default density styles are duplicated"

It looks like the mdc-button density styles are missing. I did touch those styles a bit because I found that button density was under "unmigrated-component-densities" and icon-button, fab were just missing. I'm not sure why the test is failing though. If anything my change should have fixed it since it added the missing mixins

@crisbeto
Copy link
Member

I would have to dig into why that test is failing, but from what I can tell, it is related to MDC's styles, because excluding this mixin fixes the test failure https://github.com/angular/components/blob/main/src/material/button/_icon-button-theme.scss#L55. Maybe it's being generated under the wrong selector?

@andrewseguin
Copy link
Contributor

Yeah I remember getting that when I had the wrong names in private-check-duplicate-theme-styles

@mmalerba mmalerba force-pushed the mdc-snack-bar branch 3 times, most recently from 7cf6931 to 92e752b Compare August 15, 2022 17:28
@mmalerba
Copy link
Contributor Author

mmalerba commented Aug 15, 2022

Based on my understanding of the test, it looks like @include angular-material-density(0); is somehow producing a different result for mat-icon-button than @include angular-material-theme((color: null));. This is odd because it produces the same result for every other component

@mmalerba
Copy link
Contributor Author

Mystery solved: the function we had to compare CSS output didn't know how to handle comments, which was causing it to say that the outputs weren't equivalent

@mmalerba mmalerba force-pushed the mdc-snack-bar branch 2 times, most recently from 3fc17e4 to 8bbf672 Compare August 16, 2022 00:32
Old implementation is still available under @angular/material/legacy-snack-bar

BREAKING CHANGE:
- DOM and CSS classes for mat-snack-bar have changes
- Typescript API is largely the same but may have minor differences
- See the MDC migration guide for more information about the changes and
  how to migrate your app (TODO: link when available)
Copy link
Contributor

@wagnermaciel wagnermaciel left a comment

Choose a reason for hiding this comment

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

lgtm

@andrewseguin andrewseguin added the action: merge The PR is ready for merge by the caretaker label Aug 16, 2022
@mmalerba mmalerba merged commit 950d437 into angular:main Aug 16, 2022
@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 Sep 16, 2022
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.

None yet

4 participants