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

Fix merge button #229

Merged
merged 16 commits into from
Aug 24, 2018
Merged

Fix merge button #229

merged 16 commits into from
Aug 24, 2018

Conversation

shawnbot
Copy link
Contributor

@shawnbot shawnbot commented Aug 23, 2018

This fixes a regression with the MergeButton demo reported on Spectrum. The problem appears to be that we forgot to replace the <Box position="absolute"> with <Absolute>.

While I was in the inspector I noticed that some styles were being applied via the style prop, which Emotion ignores. I switched these to use the css prop (from system-components) so that the styles are handled by Emotion, and upgraded the down arrow in our MergeButton component with a styled('span') that gets the same CSS.

I've also eliminated all of the utility classes from that demo! 🎉

@shawnbot shawnbot changed the base branch from master to release-2.0.1-beta August 23, 2018 21:16
Copy link

@emplums emplums 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! These really needed some cleaning up 🙈

@vercel
Copy link

vercel bot commented Aug 24, 2018

This pull request is automatically deployed with Now.

To access deployments, click Details below or on the icon next to each push.

@shawnbot
Copy link
Contributor Author

FYI, this is what the MergeButton looks like now:

image

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.

2 participants