Skip to content
This repository has been archived by the owner on Jan 13, 2025. It is now read-only.

feat(typography): add feature targeting for styles #4305

Merged
merged 3 commits into from
Jan 29, 2019
Merged

feat(typography): add feature targeting for styles #4305

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b5a7814
feat(typography): add feature targeting for styles
mmalerba Jan 29, 2019
9206472
WIP: fix formatting
mmalerba Jan 29, 2019
183ba06
Merge branch 'master' into sass-2nd-pass
kfranqueiro Jan 29, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 1 addition & 4 deletions packages/mdc-button/_mixins.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,10 +190,7 @@
$feat-structure: mdc-feature-create-target($query, structure);
$feat-typography: mdc-feature-create-target($query, typography);

@include mdc-feature-targets($feat-typography) {
@include mdc-typography(button);
}

@include mdc-typography(button, $query);
@include mdc-ripple-surface($query);

@include mdc-feature-targets($feat-structure) {
Expand Down
67 changes: 46 additions & 21 deletions packages/mdc-typography/_mixins.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,53 +20,78 @@
// THE SOFTWARE.
//

@import "@material/feature-targeting/functions";
@import "@material/feature-targeting/mixins";
@import "./variables";

@mixin mdc-typography-base {
@each $key, $value in $mdc-typography-base {
#{$key}: $value;
@mixin mdc-typography-base($query: mdc-feature-all()) {
$feat-typography: mdc-feature-create-target($query, typography);

@include mdc-feature-targets($feat-typography) {
@each $key, $value in $mdc-typography-base {
#{$key}: $value;
}
}
}

@mixin mdc-typography($style) {
@mixin mdc-typography($style, $query: mdc-feature-all()) {
$feat-typography: mdc-feature-create-target($query, typography);
$style-props: map-get($mdc-typography-styles, $style);

@if not map-has-key($mdc-typography-styles, $style) {
@error "Invalid style specified! #{$style} doesn't exist. Choose one of #{map-keys($mdc-typography-styles)}";
}

@each $key, $value in $style-props {
#{$key}: $value;
@include mdc-feature-targets($feat-typography) {
@each $key, $value in $style-props {
#{$key}: $value;
}
}
}

// Element must be `display: block` or `display: inline-block` for this to work.
@mixin mdc-typography-overflow-ellipsis {
text-overflow: ellipsis;
white-space: nowrap;
overflow: hidden;
@mixin mdc-typography-overflow-ellipsis($query: mdc-feature-all()) {
$feat-structure: mdc-feature-create-target($query, structure);

@include mdc-feature-targets($feat-structure) {
text-overflow: ellipsis;
white-space: nowrap;
overflow: hidden;
}
}

@mixin mdc-typography-baseline-top($distance) {
display: block;
margin-top: 0;
/* @alternate */
line-height: normal;
@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.

display: block;
margin-top: 0;
/* @alternate */
line-height: normal;
}

&::before {
@include mdc-typography-baseline-strut_($distance);
@include mdc-feature-targets($feat-structure) {
@include mdc-typography-baseline-strut_($distance);

vertical-align: 0;
vertical-align: 0;
}
}
}

@mixin mdc-typography-baseline-bottom($distance) {
margin-bottom: -1 * $distance;
@mixin mdc-typography-baseline-bottom($distance, $query: mdc-feature-all()) {
$feat-structure: mdc-feature-create-target($query, structure);

@include mdc-feature-targets($feat-structure) {
margin-bottom: -1 * $distance;
}

&::after {
@include mdc-typography-baseline-strut_($distance);
@include mdc-feature-targets($feat-structure) {
@include mdc-typography-baseline-strut_($distance);

vertical-align: -1 * $distance;
vertical-align: -1 * $distance;
}
}
}

Expand Down
29 changes: 26 additions & 3 deletions test/scss/feature-targeting.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,33 @@
@import "@material/feature-targeting/functions";
@import "@material/button/mixins";
@import "@material/checkbox/mixins";
@import "@material/radio/mixins";
@import "@material/feature-targeting/functions";
@import "@material/menu/mixins";
@import "@material/radio/mixins";
@import "@material/typography/mixins";

// Button

@include mdc-button($query: mdc-feature-any());

// Checkbox

@include mdc-checkbox($query: mdc-feature-any());
@include mdc-radio($query: mdc-feature-any());

// Menu

@include mdc-menu($query: mdc-feature-any());

// Radio

@include mdc-radio($query: mdc-feature-any());

// Typography

@include mdc-typography-base($query: mdc-feature-any());
@include mdc-typography(button, $query: mdc-feature-any());
@include mdc-typography-overflow-ellipsis($query: mdc-feature-any());

div {
@include mdc-typography-baseline-top(0, $query: mdc-feature-any());
@include mdc-typography-baseline-bottom(0, $query: mdc-feature-any());
}