Multiple Queries #39
Multiple Queries #39
Conversation
Add test case to check for matching `queries` prop
- changes proptypes to make query optional and to accept the `queries`
prop
- creates a list of MatchMedia objects for the `queries` prop
- update the `match` property of state to contain an object in the
format of
```
{
[mq name: string]: [matches: bool],
}
```
|
I'd love a review - happy to make any needed changes or test cases. Thanks! |
|
Is anything blocking this PR from being merged? |
modules/Media.js
Outdated
| name: mq, | ||
| qs: json2mq(queries[mq]), | ||
| })) | ||
| this.mediaQueryList = queries.map(mq => ({ |
There was a problem hiding this comment.
In the case of a single query, this.mediaQueryList is an actual MediaQueryList object. In the case of multiple queries, it's an array. I'd prefer it to be consistent.
There was a problem hiding this comment.
understood and updated. Thanks!
Previously, mediaQueryList could have multiple types. Now, it's always an array.
|
Would love to see this get merged, I literally just made the same thing and then saw this PR 😆 @mjackson how would you feel about adding context here, and the ability to grab a breakpoint by using a HoC like this: import React from 'react'
import PropTypes from 'prop-types'
import { CHANNEL } from './MediaQueryProvider'
export default function withMatchedQueries(WrappedComponent) {
const Component = (props, context) => {
return <Component matchedQueries={context[CHANNEL]} {...props} />
}
Component.contextTypes = {
[CHANNEL]: PropTypes.object,
}
return Component
}I can create another PR after this one gets merged if you are interested 😁 |
modules/Media.js
Outdated
| ]).isRequired, | ||
| ]), | ||
| queries: PropTypes.shape({ | ||
| [PropTypes.string]: PropTypes.oneOfType([ |
There was a problem hiding this comment.
This isn't valid. You can't use PropTypes.string as the key of an object because it will just be converted to a string. Instead, let's use:
queries: PropTypes.objectOf(
PropTypes.oneOfType(...)
)To reduce on code duplication, let's also create a type for the oneOfType and call it queryType.
const queryType = PropTypes.oneOfType(...)Then we can reuse that in both propTypes.query and propTypes.queries.
There was a problem hiding this comment.
understood, I've been using flow too much 🙊
modules/Media.js
Outdated
| this.mediaQueryList = [ | ||
| { | ||
| name: 'match', | ||
| mm: window.matchMedia(query), |
There was a problem hiding this comment.
The object returned from matchMedia is a MediaQueryList, so let's call it something like mediaQueryList or queryList instead of mm.
There was a problem hiding this comment.
I opted for mediaQueryList. updated.
modules/Media.js
Outdated
| this.mediaQueryList = window.matchMedia(query) | ||
| this.mediaQueryList.addListener(this.updateMatches) | ||
| if (query) { | ||
| this.mediaQueryList = [ |
There was a problem hiding this comment.
This is an array that we created. It's not really a MediaQueryList, just something we made to hold a bunch of media queries. Let's call it this.queries.
There was a problem hiding this comment.
makes sense, updated
`mediaQueryList` was not actually a `MediaQueryList`, so name it appropriately.
`mm` _is_ an acutal `MediaQueryList`, so name it appropriately.
| PropTypes.arrayOf(PropTypes.object.isRequired) | ||
| ]).isRequired, | ||
| query: queryType, | ||
| queries: PropTypes.objectOf(queryType), |
|
Thanks for being so responsive, @VinSpee! :D |
|
thanks for trusting me with such a big feature and for the detailed code reviews! Happy to see it merged. |
|
@mjackson Can you publish a new version with currently introduced features? |
Adds support for multiple queries as outlined in #38