-
Notifications
You must be signed in to change notification settings - Fork 308
add basic suport for mediaQuery aliases #203
Conversation
added functions: Config.setMediaQueryAlias(alias: string, query: string) Config.getMediaQueryByAlias(alias: string) Config.isMediaQueryAlias(alias: string)
Cool! Couple things to note:
@alexlande thougts on adding this to Radium? is it worth adding if computed properties solve the same issue with no additional code on Radium's side? Unfortunately I am inclined to say no. |
This is true and it's probably not a common way to use Radium but with the computed properties we have no clear way no change the media queries after the style objects generated.
Yes, it's perfectly make sense to implement a way that probably will be standard and i would happily do that but felt unnecessary to make regex match and string replace on every resolve when in the most cases we just need a named media query like we had in the older versions. On the other hand it would make the API more standard so i can agree whit this. |
@@ -7,6 +7,7 @@ var ExecutionEnvironment = require('exenv'); | |||
var _matchMediaFunction = ExecutionEnvironment.canUseDOM && | |||
window && | |||
window.matchMedia; | |||
var _mediaQueryAliases = new Map(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure we should be using Map
s in the Radium core yet. They require more polyfill that what is needed for just React. So far we've only required the same ES5 polyfills, and let Babel do the rest.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, a plain object would do.
Interesting, I've not thought about changing media queries at runtime.
When |
}, | ||
|
||
setMediaQueryAlias (alias: string, query: string) { | ||
if (alias.indexOf('@') !== 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should make checks like this for development only using if (__DEV__) { ... }
and some kind of webpack rule.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, and it seems pretty easy to add with webpack.
Still not sure about this. In principle I like it, but computed properties (or the more ugly Changing media queries at runtime is interesting, but an edge case. You could feasibly do it now by defining the custom media query styles in your var styles = {
customMediaQuery: { }
}; render() {
styles[globalMediaQueryVariable] = styles.customMediaQuery;
return (...);
} |
I guess you wouldn't want to mutate |
Yes, i'd like to "just pass in the customMediaQuery object into the component's style prop along with the other styles and let Radium resolve it" but it's really not the goal to solve my personal use case. If you don't feel like Radium should have some functionality like |
@azazdeaz: Yeah, sorry, didn't mean you specifically, but "person [x] who might have that use case." I mostly meant that there is an option (that's sort of ugly but not too bad) if someone has that requirement. I'm thinking we don't need this in Radium unless we find another compelling use case. It's pretty doable with the available tools already. |
I tend to agree, especially since this adds 3 methods to the API. Thanks On Thu, Jun 11, 2015, 10:27 AM Alex Lande notifications@github.com wrote:
|
Regards this and this comment i implemented a possible way to use named media queries.
It's pretty simple:
and inside the
resolveMediaQueryStyles()
function it replaces the the alias to the queryIn our use case we saving the styles into JSON so it's shrinks the file size and makes it easier to edit the mediaQueries later.
What do you think?