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

Reusable Media Queries with String Styles wasn't working #533

Merged
merged 4 commits into from
Jan 12, 2018

Conversation

ARChilton
Copy link
Contributor

I found an issue with the Reusable Media Queries with String Styles example on the website, however when I add min-width: as shown in the pull request it began to work.

I am not sure if the changes I have made are a sufficient fix to update the website but I have updated media-queries.md

Thanks,
Adam

Was missing min-width in the media query construction
@emmatown emmatown self-requested a review January 11, 2018 12:47
@@ -95,7 +95,7 @@ const mq = Object.keys(breakpoints).reduce(
typeof breakpoints[label] === 'string' ? '' : 'px'
accumulator[label] = cls =>
css`
@media (${breakpoints[label] + suffix}) {
@media (min-width:${breakpoints[label] + suffix}) {
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the PR! I rewrote this while changing the docs and should've tested it better, this change breaks for the string case though, could you change it to something like this?

let prefix = typeof breakpoints[label] === 'string' ? '' : 'min-width:'
// in the media query interpolation
prefix + breakpoints[label] + suffix

emmatown and others added 3 commits January 12, 2018 08:35
Updated with reference to the comments made by @mitchellhamilton.
The Prefix is now determined by whether the value is a string or not
Reusable Media Queries with String Styles wasn't working:2
@ARChilton
Copy link
Contributor Author

ARChilton commented Jan 12, 2018

I'm new to contributing to other projects so I hope I have done the right things GitHub wise. But it looks like the changes made with reference to your comment @mitchellhamilton is there.
Thank you for your work.

Copy link
Member

@emmatown emmatown left a comment

Choose a reason for hiding this comment

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

Thanks!!!

@emmatown emmatown merged commit b3fa250 into emotion-js:master Jan 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants