Skip to content

Test style rules with media queries options #56

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

Merged

Conversation

yanawaro
Copy link
Contributor

@MicheleBertoli
As to #52
Enhancements to support media queries rules is here. I tried to use less code as possible 😀. Don't know if you have better patterns on checking the options (I don't know if you like using objects to define functions to handle or else 😂). For now it looks clean, but if other more options (like finding rules for :hover as mentioned further in the issue is implemented.

Also using options as mention earlier like { media: { 'max-width': '768px' } } may not be easy. It requires more work, I read the css parser docs and it doesn't parse the media syntax 😂. So maybe right now just use simple strings to match media rules, because it could easily support multiple rules too like (min-width: 480px) and (max-width: 920px)

If its ok, then I should add some documentation 👍

@MicheleBertoli
Copy link
Member

🙌 This is awesome, thanks @yanawaro.

I tried to use less code as possible

Simple and effective 😻

Copy link
Member

@MicheleBertoli MicheleBertoli left a comment

Choose a reason for hiding this comment

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

As I said, I really love how you implemented this functionality (and I'm happy to merge this as it is).
I added a couple of comments/questions just to get your feedback.
Thank you very much.

const getRules = (ast, classNames, options) => {
let rules = ast.stylesheet.rules
if (options.media) {
rules = rules
Copy link
Member

Choose a reason for hiding this comment

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

Two questions for you:

  1. What do you think about moving this into a separate function?

Something like:

const rules = options.media ? getMediaRules(ast, options.media) : ast.stylesheet.rules
  1. What if we make this function even more generic?
const getAtRules = (ast, options) => Object.keys(options).map(option =>
  ast.stylesheet.rules.filter(rule => rule.type === option && rule[option] === options[option])
    .map(rule => rule.rules)
    .reduce((acc, rules) => acc.concat(rules), [])
)

It would support @supports for free :)

Copy link
Contributor Author

@yanawaro yanawaro Jul 26, 2017

Choose a reason for hiding this comment

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

Yep, both looks good 👍 . I've been thinking in this way before too. If it could dynamically use the options keys for filtering rules. But at first i wasn't sure if handling others than @media are the same. If number 2 is apply, is number 1 gonna look like this?

const rules = Object.keys(options).length ? getAtRules(ast, options.media) : ast.stylesheet.rules

Copy link
Member

Choose a reason for hiding this comment

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

I'd say:

const rules = options ? getAtRules(ast, options) : ast.stylesheet.rules

If you like it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because I already set the default to an empty object, it would always be true. 😱

function toHaveStyleRule(received, property, value, options = {}) {

or change its default to false I'm okay, or remove the default 😂, if you like.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, you are right - I missed that.
I think we can avoid setting a default value, just (received, property, value, options) would work 👍

rules = rules
.filter(rule => rule.type === 'media' && rule.media === options.media)
.map(rule => rule.rules)
.reduce((a, b) => a.concat(b), [])
Copy link
Member

Choose a reason for hiding this comment

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

What about acc, rules instead of a, b?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No problem. Yeah, variable names should be meaningful with the context. 😂

Copy link
Member

Choose a reason for hiding this comment

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

👍

@yanawaro
Copy link
Contributor Author

@MicheleBertoli It's been quiet, are you waiting for me to submit changes? Sorry if you are, then I misunderstood that you wrote

I added a couple of comments/questions just to get your feedback.

@MicheleBertoli
Copy link
Member

Oh yeah @yanawaro, my feedback was non-mandatory but since you agreed it'd be awesome if you could submit the changes.
I can't wait to get this merged :)
Thank you very much.

@yanawaro
Copy link
Contributor Author

@MicheleBertoli Ok, so what do you suggest for submitting changes?

  • commit as a new commit
  • amend
  • or else 😂

@MicheleBertoli
Copy link
Member

Anything that works for you is fine @yanawaro.
Feel free to push a new commit, I'll squash and merge the PR.
Thanks!

@yanawaro
Copy link
Contributor Author

@MicheleBertoli There,

  • I discovered that it has to have another array reduce at the end.
  • Added documentation

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.

2 participants