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(top-app-bar): Move comment line to appropriate section #4610

Merged
merged 4 commits into from
May 3, 2019
Merged

fix(top-app-bar): Move comment line to appropriate section #4610

merged 4 commits into from
May 3, 2019

Conversation

ALMaclaine
Copy link
Contributor

Move MDCTopAppBar mixin that is private from public to private section

fixes #4609

@@ -103,32 +132,3 @@
}
}
}

Copy link
Contributor

@abhiomkar abhiomkar Apr 18, 2019

Choose a reason for hiding this comment

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

It was intentional to keep all private mixins at the bottom of the file. Any reason for change of its location?

Copy link
Contributor

Choose a reason for hiding this comment

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

In that case, doesn't it make sense to move // private comment lines above mdc-top-app-bar-mobile-breakpoint_ mixin? This'll avoid touching git history.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea, that would be better, I'll adjust it.

@ALMaclaine
Copy link
Contributor Author

It's in the public section.

https://github.com/material-components/material-components-web/blob/master/packages/mdc-top-app-bar/_mixins.scss#L73

I moved it to the private section.

Copy link
Contributor

@abhiomkar abhiomkar left a comment

Choose a reason for hiding this comment

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

It is intentional to keep private mixins at the bottom of sass file.

@ALMaclaine
Copy link
Contributor Author

@abhiomkar There's a private mixin in the public section.

@ALMaclaine ALMaclaine closed this Apr 23, 2019
Move MDCTopAppBar mixin that is private from public to private section

fixes #4609
@ALMaclaine ALMaclaine reopened this Apr 23, 2019
@ALMaclaine
Copy link
Contributor Author

@abhiomkar Okay, I believe I adjusted it correctly.

@abhiomkar abhiomkar changed the title fix: Fix MDCTopAppBar mixin to private fix(top-app-bar): Move comment line to appropriate section May 3, 2019
@abhiomkar abhiomkar merged commit 3e36555 into material-components:master May 3, 2019
@ALMaclaine ALMaclaine deleted the fix/MDCTopAppBarMovePrivateMixinToPrivateSection branch May 4, 2019 08:34
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.

MDCTopAppBar has private mixin in public section
3 participants