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(typography): add feature targeting for styles #4305

Merged
merged 3 commits into from
Jan 29, 2019

Conversation

mmalerba
Copy link
Collaborator

No description provided.

packages/mdc-button/_mixins.scss Outdated Show resolved Hide resolved
@mixin mdc-typography-baseline-top($distance, $query: mdc-feature-all()) {
$feat-structure: mdc-feature-create-target($query, structure);

@include mdc-feature-targets($feat-structure) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

why not include all the styles inside single @include block?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I remember @kfranqueiro mentioning on another PR (can't remember where exactly) that he preferred this:

.some-selector {
  @include mdc-feature-targets($feat-structure) {
    ...
  }
}

.some-other-selector {
  @include mdc-feature-targets($feat-structure) {
    ...
  }
}

over this:

@include mdc-feature-targets($feat-structure) {
  .some-selector {
    ...
  }

  .some-other-selector {
    ...
  }
} 

I agree that this makes it easier to read the file because you can follow the selectors down first, and then worry about what feature you're targetting at the very end

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah ok. Thanks for explaining!

Yes, packaging feature related styles (ie, structure) into one block might be good for readability. But if you think it is consistent with other files this looks good.

WDYT @kfranqueiro ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, this was based on a comment I made on one of the other PRs. IIRC I specifically commented in a case where writing selectors nested within feature-targets blocks ended up causing redundant selectors in output compared to before, so this also serves to discourage that from happening, as well as just more closely mirroring how you'd write CSS if you were to ignore the feature-targets stuff.

@mmalerba
Copy link
Collaborator Author

I made a PR to update the stylelint config: #4307

@kfranqueiro kfranqueiro merged commit 8691cf8 into material-components:master Jan 29, 2019
@jamesmfriedman jamesmfriedman mentioned this pull request Feb 5, 2019
24 tasks
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.

4 participants