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

fix(button): remove margin added by safari #5169

Merged
merged 6 commits into from
Jan 29, 2020
Merged

fix(button): remove margin added by safari #5169

merged 6 commits into from
Jan 29, 2020

Conversation

tw15egan
Copy link
Collaborator

@tw15egan tw15egan commented Jan 24, 2020 •
edited
Loading

Closes #5155

Removes margin placed on button by Safari

Changelog

New

  • Safari specific selector to remove default borders on button

Testing / Reviewing

Ensure there is no 2px margin when viewing buttons on Safari.

@tw15egan tw15egan requested a review from a team as a code owner January 24, 2020 00:19
@ghost ghost requested review from asudoh and emyarod January 24, 2020 00:19
@tw15egan tw15egan mentioned this pull request Jan 24, 2020
Closed
2 tasks
@netlify
Copy link

netlify bot commented Jan 24, 2020

Deploy preview for carbon-elements ready!

Built with commit 9e90f22

https://deploy-preview-5169--carbon-elements.netlify.com

@netlify
Copy link

netlify bot commented Jan 24, 2020

Deploy preview for the-carbon-components ready!

Built with commit 9e90f22

https://deploy-preview-5169--the-carbon-components.netlify.com

@netlify
Copy link

netlify bot commented Jan 24, 2020

Deploy preview for carbon-components-react failed.

Built with commit 9e90f22

https://app.netlify.com/sites/carbon-components-react/deploys/5e2a3805e57ab30008a455b8

@netlify
Copy link

netlify bot commented Jan 24, 2020 •
edited
Loading

Deploy preview for carbon-elements ready!

Built with commit b0fdec3

https://deploy-preview-5169--carbon-elements.netlify.com

@netlify
Copy link

netlify bot commented Jan 24, 2020 •
edited
Loading

Deploy preview for the-carbon-components ready!

Built with commit b0fdec3

https://deploy-preview-5169--the-carbon-components.netlify.com

@netlify
Copy link

netlify bot commented Jan 24, 2020 •
edited
Loading

Deploy preview for carbon-components-react failed.

Built with commit b0fdec3

https://app.netlify.com/sites/carbon-components-react/deploys/5e30c2f09ce9d40008d8bc92

asudoh
asudoh reviewed Jan 24, 2020
View reviewed changes
Copy link
Contributor

@asudoh asudoh left a comment

Choose a reason for hiding this comment

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

Thank you for jumping in @tw15egan! Just wondered if the Safari margin stuff should be in button reset style. Applying this to .bx--btn is fine, though.

@tw15egan
Copy link
Collaborator Author

tw15egan commented Jan 28, 2020 •
edited
Loading

Just wondered if the Safari margin stuff should be in button reset style.

I just want to make sure users who are not using our reset still get the safari fix. We could put in both places, I guess?

@asudoh
Copy link
Contributor

asudoh commented Jan 28, 2020

Yes both will make sense. Thanks @tw15egan for clarifying!

@tw15egan
Copy link
Collaborator Author

@asudoh updated, let me know if this works 👍

@asudoh
Copy link
Contributor

asudoh commented Jan 28, 2020

Thank you for the update @tw15egan! Looks basically good, just one question; Do we want margin: 0 regardless of non-Safari future UA style sheet change, or do we have any reason to limit the change to Safari only...?

@tw15egan
Copy link
Collaborator Author

Hmm, that's a good point. I guess we might as well just remove all margin from buttons, I can update it to that.

asudoh
asudoh approved these changes Jan 29, 2020
View reviewed changes
Copy link
Contributor

@asudoh asudoh left a comment

Choose a reason for hiding this comment

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

Thanks @tw15egan for the update!

emyarod
emyarod approved these changes Jan 29, 2020
View reviewed changes
Copy link
Member

@emyarod emyarod left a comment

Choose a reason for hiding this comment

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

looks good to me, the margins are now reset in safari

image

@tw15egan tw15egan merged commit 2ce57b3 into carbon-design-system:master Jan 29, 2020
@tw15egan tw15egan deleted the remove-safari-button-margin branch January 29, 2020 17:56
@joshblack joshblack mentioned this pull request Jan 31, 2020
Closed
@joshblack joshblack mentioned this pull request Feb 25, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emyarod emyarod emyarod approved these changes

@asudoh asudoh asudoh approved these changes

Assignees
No one assigned
Labels
status: ready to merge 🎉
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Need to remove Safari's automatic margins around buttons
3 participants
@tw15egan @asudoh @emyarod