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

Components: remove global CSS reset in button #37026

Merged
merged 8 commits into from
Oct 28, 2019

Conversation

simison
Copy link
Member

@simison simison commented Oct 24, 2019

Changes proposed in this Pull Request

Remove button CSS reset from the button component and add it to Calypso CSS reset stylesheet.

I looked at each reset rule separately —

Needed

  • background: this reset is needed in the component. Only related style .button sets is background-color.

Not needed

  • border: the shorthand sets border-width, border-style, and border-color and each of those already is in the .button. Should this still be included in case someone removes one of those in the future?

Were already in .button and can be removed as duplicate

  • outline
  • padding
  • font-size
  • -webkit-appearance
  • appearance
  • vertical-align

We cannot use all:unset rule here since it lacks IE support: https://developer.mozilla.org/en-US/docs/Web/CSS/all#Browser_compatibility :-(

Testing instructions

  • Confirm Button components don't change appearance across Calypso
  • Confirm <button> elements without .button class don't change
  • Lastly, check http://calypso.localhost:3000/devdocs/design/button

@matticbot
Copy link
Contributor

@simison simison marked this pull request as ready for review October 24, 2019 13:57
@simison simison added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. Components and removed [Status] In Progress labels 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.

@simison simison self-assigned this Oct 24, 2019
@simison simison requested a review from a team October 24, 2019 14:06
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.

This reset style is needed to make <button> elements unstyled by default, and make them look as close to a <div> as possible.

Removing it breaks a lot of places where we use <button> to create accessible clickable things that don't look like a classic button:

Media Library Items: (notice the unwanted border)
Screenshot 2019-10-25 at 09 10 11

Site Icon: (broken InfoPopover icon and clickable site icon image)
Screenshot 2019-10-25 at 09 09 41

Close buttons in Notices:
Screenshot 2019-10-25 at 09 08 04

Change Username link in Login:
Screenshot 2019-10-25 at 09 07 47

A better solution would be to move the reset to the reset.scss stylesheet. It's really a reset rather than styling specific to Calypso design.

@simison
Copy link
Member Author

simison commented Oct 25, 2019

Thanks for having a look Jarda! Yepp, I realized the same yesterday, only set this ready for review little too early. 😁

Fixed!

I'd leave background reset directly in the component as well since it's good to be defensive in re-usable components in unknown environments.

@simison simison requested a review from jsnajdr October 25, 2019 07:21
@simison simison force-pushed the update/components-button-css-reset branch from 055bf45 to b4c920b Compare October 25, 2019 07:48
@simison simison requested a review from a team October 25, 2019 08:02
Copy link
Contributor

@ockham ockham left a comment

Choose a reason for hiding this comment

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

This appears to be working well for me.

I modified the Changelog a bit (see) and asked one question.

All in all, looking good to 🚢

...handled already by Autoprefixer.
...handled already by Autoprefixer.
@simison simison merged commit dd85abc into master Oct 28, 2019
@simison simison deleted the update/components-button-css-reset branch October 28, 2019 09:33
@matticbot matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Oct 28, 2019
@simison
Copy link
Member Author

simison commented Oct 28, 2019

Here's a follow-up PR to further re-work some of the button CSS: #37103

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