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.

Sign up for GitHub

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

Jump to bottom

chore(typography): Remove the adjust margin feature #2464

Merged
merged 6 commits into from
Mar 27, 2018
Merged

chore(typography): Remove the adjust margin feature #2464

merged 6 commits into from
Mar 27, 2018

Conversation

lynnmercier
Copy link
Contributor

Fixes #2463

@codecov-io
Copy link

codecov-io commented Mar 27, 2018 •
edited
Loading

Codecov Report

Merging #2464 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2464   +/-   ##
=======================================
  Coverage   98.91%   98.91%           
=======================================
  Files         104      104           
  Lines        4234     4234           
  Branches      534      534           
=======================================
  Hits         4188     4188           
  Misses         46       46

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a99ce40...91e2e8e. Read the comment docs.

kfranqueiro
kfranqueiro suggested changes Mar 27, 2018
View reviewed changes
Copy link
Contributor

@kfranqueiro kfranqueiro left a comment

Choose a reason for hiding this comment

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

Note that the Top App Bar demo page is currently using the mdc-typography--adjust-margin class, so we'll have to remove that and put in some demo styles.

kfranqueiro
kfranqueiro reviewed Mar 27, 2018
View reviewed changes
demos/top-app-bar.html
<div>
<input type="checkbox" id="prominent-checkbox"/>
<label for="prominent-checkbox">Prominent</label>
</div>
</div>
<div class="demo-col">
<h4 class="mdc-typography--subheading2 mdc-typography--adjust-margin">Short Top App Bar Specific Options</h4>
<span class="mdc-typography--subheading2 demo-options-heading">Short Top App Bar Specific Options</span>
Copy link
Contributor

Choose a reason for hiding this comment

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

This change makes me realize something, related to this PR as well as the task to redefine the typography scales...

If we're going to be introducing typography scales for Headline 1-6, I would assume they're naturally going to often align with h1 to h6. These elements commonly have default margins set by the user agent, which might not be appropriate when combined with our styles. Are we sure we don't want to override those with saner defaults? Or do we think we'd override those in all cases they're used, perhaps, thus obviating the need for adjust-margin anyway?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think typography should only handle font properties (font-family, font-size, letter-spacing, etc) which can be displayed in any HTML element. We should not assume that @mdc-typography mixin is applied to a h3 element.

kfranqueiro
kfranqueiro suggested changes Mar 27, 2018
View reviewed changes
Copy link
Contributor

@kfranqueiro kfranqueiro left a comment

Choose a reason for hiding this comment

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

This PR should also remove the use of the mdc-typography--adjust-margin class from typography.html... it'll be interesting to see if there's any fallout from doing that in terms of the page's appearance.

kfranqueiro
kfranqueiro approved these changes Mar 27, 2018
View reviewed changes
Copy link
Contributor

@kfranqueiro kfranqueiro left a comment

Choose a reason for hiding this comment

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

This needs a BREAKING CHANGE: message explaining the removal of the --adjust-margin modifier class and the adjust-margin mixin.

One more wording change suggestion, otherwise LGTM.

packages/mdc-typography/README.md Outdated
> **A note about `mdc-typography-overflow-ellipsis`**, `mdc-typography-overflow-ellipsis` should only be used if the element is `display: block` or `display: inline-block`.

#### `$style` Values

These styles can be used as the `$style` argument for `mdc-typography` and `mdc-typography-adjust-margin` mixins.
These styles can be used as the `$style` argument for `mdc-typography` mixins.
Copy link
Contributor

Choose a reason for hiding this comment

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

Fix wording: These styles can be used as the $style argument for the mdc-typography mixin.

@lynnmercier lynnmercier merged commit a12f320 into master Mar 27, 2018
lynnmercier added a commit that referenced this pull request Mar 27, 2018
@lynnmercier
chore(typography): Remove the adjust margin feature (#2464)
3f23821 
BREAKING CHANGE: Removes the `mdc-typography--adjust-margin` CSS class and the `mdc-typography-adjust-margin` Sass mixin
@lynnmercier lynnmercier deleted the chore/typography-margin branch May 31, 2018 22:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kfranqueiro kfranqueiro kfranqueiro approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@lynnmercier @codecov-io @kfranqueiro