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

Calypso CSS: refactor CSS reset and base styles #37020

Closed
wants to merge 7 commits into from

Conversation

simison
Copy link
Member

@simison simison commented Oct 24, 2019

Goal: scope CSS reset and base calypso styles so that it's possible to exclude them from /gutenboarding section.

Specifically, I'm addressing these two issues from #36170 (comment):

  • CSS reset stylesheet
  • styles for form elements, as we apply Calypso styling even to plain input or select elements that don’t have any class names

Here are some Calypso overrides previous "Gutenberg in Calypso" had to do.

Changes proposed in this Pull Request

  • Move scss partials to client/assets/stylesheets/layout, leaving client/assets/stylesheets/*.scss only for entry files
  • Move animations partial to new layout folder, leaving shared for only scss files that don't produce any visible CSS when imported
  • Move grow animation to _animations.scss
  • Change loading order so that resets and base styles come first
  • Move Calypso styles that affect non-class selectors to a new _base.scss file, goal is to strip down generic "main" file away eventually
  • Scope reset and base -styles so that it's possible to exclude them from some views. TODO: an exact mechanism to be figured out. I'm using html.gr__calypso_localhost as a temporary example selector — it'll be something else later on.
  • Update outdated README.md

I'll PR some of this separately for easier review & testing:

Testing instructions

  • Spin up Calypso
  • Observe that base and reset styles are first in entry bundle
  • Test everything!
  • /onboarding looks great, eh?

@matticbot
Copy link
Contributor

@@ -0,0 +1,150 @@
//@mixin calypso-reset {
Copy link
Member Author

@simison simison Oct 24, 2019

Choose a reason for hiding this comment

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

We don't need these to be in mixin, but it would make things easier for #36170 and (component) consumers outside Calypso.

@@ -28,11 +28,6 @@
box-sizing: border-box;
overflow: hidden;

// I guess we dont want links to look like links.
Copy link
Member Author

@simison simison Oct 24, 2019

Choose a reason for hiding this comment

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

Moved to _base.scss but will need to check how that affects things outside .layout__content — maybe leave be for now and loop back to it later. It's the only non-class-selector here.

Copy link
Member

Choose a reason for hiding this comment

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

The only thing outside .layout__content is the Masterbar and things inside it, notably Notifications. Notifications have their own .wpnc__main a { text-decoration: none } style, so they shouldn't get hurt by this change. I think it's good and safe move.

Copy link
Member Author

Choose a reason for hiding this comment

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

PRed separately so that it's easier to test #37064

@simison simison changed the title Move things around Calypso CSS: refactor CSS reset and base styles Oct 24, 2019
@simison simison self-assigned this Oct 24, 2019
@matticbot
Copy link
Contributor

This PR does not affect the size of JS and CSS bundles shipped to the user's browser.

Generated by performance advisor bot at iscalypsofastyet.com.

Copy link
Member

@jsnajdr jsnajdr left a comment

Choose a reason for hiding this comment

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

Scope reset and base -styles so that it's possible to exclude them from some views.

I understand the need for scoping the base styles, but why reset? It would help to know the use case that requires the scoping feature. Or is it from the "someone might need it" bucket at this moment?

@@ -28,11 +28,6 @@
box-sizing: border-box;
overflow: hidden;

// I guess we dont want links to look like links.
Copy link
Member

Choose a reason for hiding this comment

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

The only thing outside .layout__content is the Masterbar and things inside it, notably Notifications. Notifications have their own .wpnc__main a { text-decoration: none } style, so they shouldn't get hurt by this change. I think it's good and safe move.

@simison simison force-pushed the update/refactor-calypso-css-reset branch from 763f7a9 to b88e1ad Compare October 25, 2019 11:12
@simison
Copy link
Member Author

simison commented Nov 25, 2019

Pretty much everything from this has been handled in separate PRs or problems worked out in other ways, so I'll close.

Calypso stylesets definetely need some more work but new PRs are better for that. :-)

@simison simison closed this Nov 25, 2019
@simison simison deleted the update/refactor-calypso-css-reset branch November 25, 2019 09:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants