-
Notifications
You must be signed in to change notification settings - Fork 3.4k
The styles for hide/show[-xx[-yy]]
are wrong
#772
Comments
Great analysis @gkalpak! I agree with most everything you wrote but I wanted to add my two cents in defense of an all powerful <div show hide-gt-md> would behave the same way as just: <div hide-gt-md> But I do think it would be useful to have Thinking about it some more as I write this, I suppose that I could achieve the same functionality by only using What do you think? |
@mzbyszynski: You definitely have some point :) As you too mention, this wouldn't provide any new functionality (everything would be achievable without the use of |
Hmm, so since this issue was created this was modified a bit.
For example: However, I don't know if I like this .. it is quite confusing. I think I actually like getting rid of the show attributes altogether ... Except maybe the one |
That means we end up with the following: [hide]:not([show]) {
display: none;
}
@media (max-width: $layout-breakpoint-sm) {
[hide-sm]:not([show]) {
display: none;
}
}
@media (min-width: $layout-breakpoint-sm) {
[hide-gt-sm]:not([show]) {
display: none;
}
}
@media (min-width: $layout-breakpoint-sm) and (max-width: $layout-breakpoint-md) {
[hide-md]:not([show]) {
display: none;
}
}
@media (min-width: $layout-breakpoint-md) {
[hide-gt-md]:not([show]) {
display: none;
}
}
@media (min-width: $layout-breakpoint-md) and (max-width: $layout-breakpoint-lg) {
[hide-lg]:not([show]) {
display: none;
}
}
@media (min-width: $layout-breakpoint-lg) {
[hide-gt-lg]:not([show]) {
display: none;
}
} Thoughts? |
Makes sense to me. I can't think of a use-case for the show-xx attributes that couldn't be accomplished using the hide-xx. |
It was suppose to work like this when I created the issue, but it didn't work as expected.
Indeed having
I am pretty sure @mzbyszynski didn't suggest keeping only the Regarding #772 (comment):
(Not only is it more verbose, but it is much less readable as well, i.e. you have to think more to find out what it does.) Considering the above, I think the selectors that should have sm: 0 < size < 600 gt-sm: 600 < size md: 600 < size < 960 gt-md: 960 < size lg: 960 < size < 1200 gt-lg: 1200 < size |
My original suggestion (the way I was thinking about it anyway; I may not have communicated it properly) was for keeping the I guess it is a trade-off: symmetry and more concise html versus simplicity and clarity of the API. Personally I think I agree more with @ajoslin. I would prioritize a simpler API over concise html. I don't mind opinionated frameworks, and I think in most cases providing one way to accomplish a given use case is better than providing multiple ways. It is generally easier to develop, test, maintain and document the former, and having only one way to do something should prevent (or at least cut down on) semantic arguments about which way is the right way to code something. But I suppose it boils down to what the design goals/priorities are for this project and the core team. I haven't found too much about what these are beyond this statement from the README:
Is having all these flavors of I could probably be convinced either way 😄, but I keep thinking of a moment when I first started using ngMaterial: I had a little trouble getting my head around the layout options, but reading @pandaiolo's comments on #494 clarified it for me... if it is a "mobile first" framework then I need to make my default design the mobile screen and add exceptions, duh! Of course, the layout system now is a lot more complex/flexible than it was when those comments were written so they may no longer apply, but putting that aside--once I understood the concept I was able to follow this approach and move on with my project instead of fighting with the framework to make it fit the way I was thinking about my application. I mention this as an example of how simplicity & having fewer options for accomplishing a task can sometimes be more help to a developer than the alternative. That's where I'm landing on this issue today anyway. Great discussion so far, and I'm looking forward to further debate! -Marc |
@gkalpak I still like the lack of show better - for the reasons that @mzbyszynski said - it is simpler, and leads to one way to do things. Your example is rather compelling when I look at it:
But then there's this very confusing counterexample that we allow the developer to do:
For this reason, I like removing the show attributes. |
@ThomasBurleson since this discussion is already in progress, we'll keep it here. |
@ajoslin: I defintely don't think it is a good isea to remove the That said, I appreciate you sharing the reasoning and thoughts behind the decisions and I totally understand you can't keep everyone satisfied. |
With your opinion @gkalpak and discussion with @rschmukler, we've decided to keep it as is. It's the best option we have right now. Thanks for the input all! |
@ajoslin, @rschmukler: I don't really understand what "It's the best option we have right now" is supposed to mean. Both @ajoslin's suggestion and mine are much better than the current one (although I prefer mine for obvious reasons). The problem with the current implementation (and the reason I opened this ticket in the first place) is that right now it is broken :) A few reasons:
(I haven't exhaustively looked for cases that don't work, but I am very confident there will be more.) I understand that the team may decide on a different approach (than what I suggested), but I believe it should be implemented correctly/bug-free. I propose you (I mean the team) write down what is the expected behaviour, so we can 1. update the docs and 2. submit issues on specific CSS bugs. I hope I don't sound like that cranky guy; I am just trying to help :) |
I didn't realize you had suggested the "show-gt-* only overrides hide" solution. I don't like this because it is not intuitive ... When I see show-gt-lg, I would think it would override hide-gt-md. |
Basically, you can think of it as It is the same story with You could achieve the same functionality with either one (thus your proposal of removing |
I'm thoroughly confused with these attributes. My sugession is to have |
@altufaltu try |
@gkalpak so I'm finally getting to this. The changes you suggested still broke in some cases. After much thought and testing, I've started over with a simpler approach. I couldn't find any problems with it. Please poke at it and let me know your thoughts! /**
* `hide-gt-sm show-gt-lg` should hide from 0px to 1200px
* `show-md hide-gt-sm` should show from 0px to 600px and >960px
* `hide-gt-md show-gt-sm` should show everywhere (show overrides hide)`
*/
// SMALL SCREEN
@media (max-width: $layout-breakpoint-sm) {
[hide-sm], [hide] {
&:not([show-sm]):not([show]) {
display: none;
}
}
}
// MEDIUM SCREEN
@media (min-width: $layout-breakpoint-sm) and (max-width: $layout-breakpoint-md) {
[hide], [hide-gt-sm] {
&:not([show-gt-sm]):not([show-md]):not([show]) {
display: none;
}
}
[hide-md]:not([show-md]):not([show]) {
display: none;
}
}
// LARGE SCREEN
@media (min-width: $layout-breakpoint-md) and (max-width: $layout-breakpoint-lg) {
[hide], [hide-gt-sm], [hide-gt-md] {
&:not([show-gt-sm]):not([show-gt-md]):not([show-lg]):not([show]) {
display: none;
}
}
[hide-lg]:not([show-lg]):not([show]) {
display: none;
}
}
// BIGGER THAN LARGE SCREEN
@media (min-width: $layout-breakpoint-lg) {
[hide-gt-sm], [hide-gt-md], [hide-gt-lg], [hide] {
&:not([show-gt-sm]):not([show-gt-md]):not([show-gt-lg]):not([show]) {
display: none;
}
}
} |
I am very happy to this being dealt with (finally 😛). Thx @ajoslin Would still be interested to know in what cases my suggestion broke :) |
@ajoslin: Haven't tested it, but "theoretically" it looks fantastic (it's indeed more complete than my "basic" suggestion). I have only left a couple on comments on incorrect comments (which might confuse future maintainers or "sourcerers"). I like the fact that it sets up a proper "priority" of each rule "family" (it might be worth documenting more explicitly). From "strongest" (beats all) to "weakest" (looser):
|
Looks great @ajoslin! 👍 |
The styling fails at the breakpoint of 600px. In the API reference demo for hide and show attributes, https://material.angularjs.org/#/layout/options, both of the md-subheaders are hidden on 600px devices. |
It works fine for me (Chrome 40 on Windows 8.1). What browser/platform are you trying this on ? |
I am testing with Chrome 40 on OS X Yosemite and Chrome 40 on Windows 7 with the same result. Sorry for not being clear. Because hide-sm displays none at max-width: 600px and hide-gt-sm displays none at min-width: 600px to max-width: 960px, there is not a way to show one and not the other at 600px. Both hide-sm and hide-gt-sm will display none at 600px. Thanks @gkalpak |
@thomasphan, I see what you mean. Indeed there is an issue at exactly 600px (and probably other breakpoints as well). |
I think that the current implementation of
hide[-xx[-yy]]
/show[-xx[-yy]]
is kind of broken and confusing (not sure if it is so by design or as a result of a bug in style declarations).Based on the current styles it seems that
show-xx-yy
can negatehide-xx-yy
, which seems to have little point (why would I want to defineshow-xx-yy
andhide-xx-yy
on the same element). Furthermore it comes to contradiction with the docs, which state thatshow[-xx[-yy]]
"negates hide" (nothide[-xx[-yy]]
).It is makes much more sense (imo) anyway:
Either show the element "by default" and use appropriate
hide-xx[-yy]
attributes to hide it on specific devices or hide the element "by default" (usinghide
) and use appropriateshow-xx[-yy]
attributes to negatehide
and make the element visible on specific devices.Mixing both approaches won't help.
That said, I believe that:
show
is unnecessary (since elements are shown (i.e. not hidden)) without needing any attribute (and overridinghide
across all devices doesn't make sense).show[-xx[-yy]]
should only negatehide
(on appropriate screen-sizes).According to the above, here are the selectors that should have a
display: none;
style per screen-size:any: 0 < size
sm: 0 < size < 600
gt-sm: 600 < size
md: 600 < size < 960
gt-md: 960 < size
lg: 960 < size < 1200
gt-lg: 1200 < size
BTW, this is related to the problem described in #764.
I hope all this makes sense, because I used
make sense
too many times...The text was updated successfully, but these errors were encountered: