-
Notifications
You must be signed in to change notification settings - Fork 53
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
Misleading API #17
Comments
Hi @sarneeh, you've got a point there, thanks for spotting it, it isn't exactly grammatically / logically correct. Though, at least in my humble opinion, duplicating the code to another function with one minor change would lead to code bloat - as we can't just take out Just changing the behavior of it wouldn't work for the same reason, I'd rather go with a second parameter in I'm interested in what the others or @morajabi think about it... |
@timhagn Obviously we do not want to change it as a minor version bump, as my suggestion is certainly a breaking change :) But I think it would be cool to consider this behavior in the next major. Also waiting for other to discuss this, thanks for your opinion 😃 |
I think we need to improve documentation instead of changing API. To solve your issue, you can generate custom breakpoints with suffix |
Of course users can build their own workarounds, @ApacheEx, and i completely concur with your incentive to improve the docs on this matter - but I still think @sarneeh, has a Point there... |
@ApacheEx Of course, I can solve my issue with a workaround, but I'm pretty sure I'm not and won't be the only one who will expect from this library this kind of option. And we're building libraries to make our lives easier, aren't we? :) I think that the change is so small compared to what already has been done here that I think we shouldn't argue if it should be done at all but how to approach this problem 😄 |
I think this is very important, currently if you perform a layout switch at a specific breakpoint using lessThan and moreThan you will have a 1px conflict zone where both sets of rules apply at the same time, yes you can write a workaround by I can't see a use case where someone would want this behaviour by default? |
@andy-hook you're running through open doors on my side - that's one more point in favor of changing the API (though I'm still for the small addition of a Boolean so it won't break any production code before |
@andy-hook Can you tell me why you would have both a |
@acnebs There are many reasons why you would structure your code this way, the method you suggest inherently relies on cascade / declaration order, by assuming code outside a A better approach is to apply style rules in size ranges, so that they only take effect when they are needed, this vastly simplifies managing complex layout as you don't have to keep resetting previously set rules, they also don't appear in your inspector and you can compose different classes together to apply responsive behaviour with complete confidence that their style code is only applied within the given media query range. This is partially achievable using |
opinion: i think and as the use, i‘ve used both |
How about this for a syntax:
Or, perhaps something like this:
|
So when I'm doing a
media.lessThan('1024px'))
I'd expect the media query to look like:@media (max-width: 1023px) { }
, because that's what the API states: lessThan. The same issue applies togreaterThan
.What I'd suggest here is:
lessThanOrEqual
/greaterThanOrEqual
lessThan
always substract1
from passed value, andgreaterThan
add1
to the valueWe could also leave the API as is and add the
-OrEqual
variations.What do you think about it?
The text was updated successfully, but these errors were encountered: