Skip to content
This repository was archived by the owner on Jan 27, 2019. It is now read-only.

Conversation

@johno
Copy link
Member

@johno johno commented Jan 27, 2015

Related to cssstats/cssstats#76 and cssstats/cssstats#102
Related to #2

I've added unit tests for media query parsing to replicate the issue referenced
above. Then, when attempting to get the test to pass I dove into how the
media queries were being handled. The current implementation doesn't hold
reference to the raw value after camel casing the key, so I don't believe
it's possible to fix this issue without changing the returned object.

So, I figured we could use this as the starting point to discuss the best
way to approach this. The first idea of mine is to slightly change the byMedia
key in the declarations object to be:

byMedia: {
  '(minWidth:12345em)': {
    raw: '(min-width:12.345em)',
    declarations: [
      // ...
    ]
  }
}

@jxnblk
Copy link
Member

jxnblk commented Jan 29, 2015

It would be possible to keep the camelCased, dot-syntax friendly key and just expose the raw value, right? Sorry, it's been a while since I've looked into this.

@johno
Copy link
Member Author

johno commented Jan 29, 2015

Yeah, absolutely. When I get a chance I'm going to dive into the cssstats ng app and see how the data is being used, where the dot-syntax comes into play, etc. As it would be ideal to keep it as in line with cssstats as possible.

My proposed solution above would also allow you to populate the media query array with the raw value (if desired):

{
  // ...
  mediaQueries: ['(min-width:12.345em)'],
  // ...
}

Which would then allow the media query display to be correct in the view (assuming it's just pulling from the aggregate). Then, when accessing the declarations for said media query you can still have access:

var mediaDeclKey = camelCase('(min-width:12.345em)');
var mediaQueryDecls = stats.declarations.byMedia(mediaDeclKey);
mediaQueryDecls.raw;  // => '(min-width:12.345em)'
mediaQueryDecls.declarations;  // => [/* ... */]

This would change the API a bit from the way it currently works, though, as it's currently structured like (going from memory here so it might be slightly incorrect):

{
  declarations: {
    byMediaQuery: {
      '(min-width:12.345em)': [ /* ... */]
    }
  }
}

I will ponder this a bit more to figure out the best approach, and cross-reference it with what's going on in cssstats.

@jxnblk
Copy link
Member

jxnblk commented Jan 29, 2015

Yeah, I kinda wanna think about this some more, and how it works with mrmrs/cssstats. Also btw, the current app doesn't use Angular, just Express.

@johno
Copy link
Member Author

johno commented Jan 29, 2015

Yeah, I'll just let this sit for a bit so we can ruminate. Orly? I need to dive into it to see what's going on in that app, as I'm currently clueless.

/me goes to deep dive into cssstats

@johno johno added the question label May 6, 2015
@johno johno mentioned this pull request Jul 28, 2015
Merged
6 tasks
@johno
Copy link
Member Author

johno commented Aug 4, 2015

Closing as this will be handled by #42

@johno johno closed this Aug 4, 2015
@johno johno deleted the handling-ems-in-media-queries branch August 4, 2015 20:40
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants