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

Use polished normalize in GlobalStyle #243

Closed
wants to merge 1 commit into from
Closed

Use polished normalize in GlobalStyle #243

wants to merge 1 commit into from

Conversation

alkamin
Copy link
Contributor

@alkamin alkamin commented Jan 15, 2020

Overview

This PR removes most of the original global styles which were derived from normalize.css and instead uses the normalize() method from polished.

It maintains the themeable rules and also keeps the body font-size rule for the 1rem => 10px equivalence.

Checklist

  • Description of PR is in an appropriate section of the changelog and grouped with similar changes if possible

Upgrade instructions

This PR shouldn't require any upgrades.

Testing Instructions

  • Check out the docs and see that everything looks the same as before

Closes #242

@alkamin alkamin force-pushed the feature/ak/use-polished-normalize branch from 6fffde2 to f739383 Compare January 15, 2020 13:50
@alkamin alkamin removed the request for review from lederer January 15, 2020 14:29
@lederer
Copy link
Contributor

lederer commented Jan 15, 2020

The original global styles are the Meyer reset, which fully strips out all styling, whereas Normalize retains some opinionated styling and normalizes it across browsers. So there may be some unintended side effects of this change.

@alkamin
Copy link
Contributor Author

alkamin commented Jan 15, 2020

Should we consider it a breaking change?

@lederer
Copy link
Contributor

lederer commented Jan 15, 2020 •
edited
Loading

That may be prudent. Also looks like we should address some specific fallout. I did a SxS comparison and noticed differences in:

  • Callout
  • Card
  • Dialog
  • FileInput
  • Heading
  • Table
  • Text
  • TextInput

Many are probably rooted in <h?> tags retaining some margin in Normalize. And the Table diff was particularly noticeable.

@alkamin
Copy link
Contributor Author

alkamin commented Jan 15, 2020 •
edited
Loading

Thank you for the thorough inspection.

I guess the big questions are:

  1. Is this worth it?
  2. If it is, should we modify the default theme to accommodate these differences

And also, if we choose to move forward we'd need to write some upgrade guidance. Probably best to keep this one un-merged for now until we answer these.

@alkamin alkamin added the Breaking Changes Used on PRs and issues to indicate SemVer impact label Jan 25, 2020
@alkamin
Copy link
Contributor Author

alkamin commented Jan 25, 2020

I'm going to close this until we can have some further discussion on it. I think that no matter how we slice it this will be considered a breaking change.

@alkamin alkamin closed this Jan 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@designmatty designmatty designmatty approved these changes

Assignees
No one assigned
Labels
Breaking Changes Used on PRs and issues to indicate SemVer impact
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Update GlobalStyle to utilize Polished Normalize
3 participants
@alkamin @lederer @designmatty