[system] Add not operator to breakpoints
#29311
Conversation
|
I have resolved the problems you mentioned, thank you! |
mbrookes
added
new feature
|
|
||
| it('should invert down for xs', () => { | ||
| expect(breakpoints.not('xs')).to.equal( | ||
| '@media not all and (min-width:0px) and (max-width:599.95px)', |
There was a problem hiding this comment.
I don't think this is correct. If it should not be applied for xs it should not be applied till (max-width:600px) no? I believe we need to handle carefully the first and last breakpoint.
@chwallen what do you think?
There was a problem hiding this comment.
@mnajdova The query has the correct result but it is confusing to read not queries imo. xs is 0px to 600px, so either we write it as not between('xs', 'sm') (which is how only currently handles keys with index 0), or not down('xs').
For a more readable query, my suggestion would be to change the implementation of only to handle the 0-key as a special case. That would change the query above to to '@media not all (max-width:599.95px)' which I find slightly more readable like so (adding the keyIndex === 0 case):
function only(key) {
const keyIndex = keys.indexOf(key);
if (keyIndex === 0) {
return down(keys[1]);
} else if (keyIndex + 1 < keys.length) {
return between(key, keys[keys.indexOf(key) + 1]);
} else {
return up(key);
}
}Otherwise, @Philipp000 handled these special cases in his initial commit which removes the not operator entirely for both the first and last breakpoint. I would modify his implementation and use between(key, key + 1) instead of constructing the inverted between query manually. The implementation is not as simple as just inverting only, but perhaps more readable.
There was a problem hiding this comment.
We can't change the implementation for only as it would be a breaking change.
Otherwise, @Philipp000 handled these special cases in his initial commit which removes the not operator entirely for both the first and last breakpoint. I would modify his implementation and use between(key, key + 1) instead of constructing the inverted between query manually. The implementation is not as simple as just inverting only, but perhaps more readable.
Would you like to try this? Whoever wants to continue the effort is more than welcomed :)
There was a problem hiding this comment.
We can't change the implementation for only as it would be a breaking change.
I don't see how it would be a breaking change by modifying only. The queries would be the same except for only('xs') which changes from @media (min-width:0px) and (max-width:599.95px) to @media (max-width:599.95px) (i.e. removing the redundant (min-width:0px) clause). If there are custom breakpoints, it is equivalently changed for the lowest breakpoint.
However, if you still consider it a breaking change, then we will skip only and focus on not.
Would you like to try this? Whoever wants to continue the effort is more than welcomed :)
I assume I can't push to this branch so either I re-open my PR and continue there or we wait for Philipp.
There was a problem hiding this comment.
I have now updated the implementation, to handle the first and last breakpoint differently. I hope, this improves the readability.
However, as @chwallen already pointed out, and as far as I'm concerned the results in the browser are identical.
Philipp000
force-pushed
the
extend-breakpoints-with-not
branch
from
0e7d2b4
to
9e6c420
Compare
Allow to easily create styles, which are the inverted version of the only operator Updated the documentation and included tests
Update the implementation of the "not" operator by using string replace
…ntly, for better readability.
hbjORbj
force-pushed
the
extend-breakpoints-with-not
branch
from
9e6c420
to
bccfaf2
Compare
hbjORbj
changed the title
not operator to breakpoints
Allow to easily create styles, which are the inverted version of the only operator
Updated the documentation and included tests
This change is related to issue #29228