News:

Want to get involved in developing SMF, then why not lend a hand on our github!

Main Menu

CSS - We can haz bugz?

Started by Antechinus, August 26, 2021, 07:18:01 PM

Previous topic - Next topic

Antechinus

Code (responsive.css) Select
.popup_window, #main_menu .popup_window, #genericmenu .popup_window {
position: relative;
z-index: 99;
width: 90%;
box-shadow: 0 3px 6px rgba(0, 0, 0, 0.5);
border: 1px solid #777;
border-radius: 7px 7px 3px 3px;
min-height: 285px !important;
max-height: 5em;
overflow-x: hidden !important;
overflow-y: scroll !important;
-webkit-overflow-scrolling: touch;
background: #fff; /* fallback for some browsers */
background-image: linear-gradient(to bottom, #e2e9f3 0%, #fff 70%);
top: 15%;
padding: 0;
margin: 0 auto;
}

1/ Everything is in an inconsistent order. It's always handy if things like display and position are first, followed by width, height, margin, padding etc, with borders and box-shadows last. It's logical, and it does save some frigging around when customising things, because you know where to look.

2/ Relative positioning is already defined on Line 2725 of index.css, and is not overriden anywhere else. It does not need to be in responsive.css.

3/ The same applies to the z-index. Which, really, should not be necessary even in index.css. Its parent element (.popup_container) has fixed positioning and a z-index anyway.

4/ The really dumb bit is this:
min-height: 285px !important;
max-height: 5em;
The only reason someone had to use !important on min-height is because they didn't look at the max-height set directly after it. And the max-height set is less than the min-height, unless font-size is greater than 57px, which it won't be. :P

TBH I think it would make sense to just set min-height to 50%, at least for menus (ie: #main_menu .popup_window and #genericmenu .popup_window). It will still look fine and will save people having to pan/scroll sometimes. It would probably make sense to set max-height to 70% or less.

5/ The only reason overflow needs !important here is because it is defined as overflow: visible !important; on Line 2724 of index.css. But, since that is default behaviour for any div it does not need to be defined in the first place. Line 2724 of index.css is superfluous and can be (and should be) removed, meaning you don't need !important here if index.css is sorted properly.

6/ Linear gradients are "to bottom" by default. You don't need to declare that. Also, 0% is implicit for the first colour stop so you don't need that either.

7/ box-shadow has 0.5 opacity, with the 0 being a waste of a byte.

Saner way of doing this:

Code (responsive.css) Select
.popup_window, #main_menu .popup_window, #genericmenu .popup_window {
top: 15%;
width: 90%;
min-height: 285px;
max-height: 70%;
margin: 0 auto;
padding: 0;
overflow-x: hidden;
overflow-y: scroll;
-webkit-overflow-scrolling: touch;
background: #fff; /* fallback for some browsers */
background-image: linear-gradient(#e2e9f3, #fff 70%);
border: 1px solid #777;
border-radius: 7px 7px 3px 3px;
box-shadow: 0 3px 6px rgba(0 , 0, 0, .5);
}
#main_menu .popup_window, #genericmenu .popup_window {
min-height: 50%;
}

And a really obvious question: why use the .popup_window class for #main_menu and #genericmenu anyway?

All that does is force you to override all the default styles for that class in index.css (so it won't bork things at > 480px wide) just after they have been defined for actual pop-ups (help, etc). Then you have to redeclare all those styles in responsive.css once width gets down to 480.

It's a tad bonkers. :P If you want a class that is invisible by default, why declare one that is going to screw things? It would make more sense to use a different class for mobile menus, that only has eye candy declared in one place (responsive.css).

PS: Stuff like this is good - https://github.com/necolas/idiomatic-css
PPS: Stuff like this is good too - https://cssguidelin.es :)

Antechinus

Excellent. Found another bug. Someone has removed the previously-existing border: 1px solid transparent; around the anchors inside drop menus, and replaced it with border: 0;

This may have seemed sensible at the time, if you know nothing about drop menus. The reason it was there is so that when you want a visble border on hover or focus the anchors will not move around!!

If there is a transparent border you can just change the border-color on hover and focus, without shoving anything out of place. Now, they move around sometimes, because no border is being replaced with a 1px border.

They don't move around if you are user hover activation, because at least said someone did include code to show a border on hover of the parent li, and by the time the Superfish animations are dealt with that's ok.

However, if you are using keyboard activation of the drop menus (and allowing that was one of the primary reasons for including Superfish in the first place) then because there is no hover on the parent li the drop menu will become visible without any border set on its child anchors.

Then, as soon as you tab to one of those anchors it will have a 1px border applied and will jump out of place.

Fortunately, this is easy to fix. Very easy. Find this:

Code (index.css - Lines 905 - 912) Select
/* Necessary to allow highlighting of 1st level while hovering over submenu. */
.dropmenu li:hover li a, .dropmenu li li a {
background: none;
padding: 0 9px;
color: #346;
border: none;
line-height: 2.2em;
}

Replace with what it used to have, which worked all the time.

/* Necessary to allow highlighting of 1st level while hovering over submenu. */
.dropmenu li:hover li a, .dropmenu li li a {
background: none;
padding: 0 9px;
color: #346;
border: 1px solid transparent;
line-height: 2.2em;
}

Hey presto, anchors will not move regardless of whether you are using hover or keyboard activation.
You're welcome.  ;)



ETA: Oh crap. And lol. Turns out the transparent border is still defined. But, it is defined earlier in index.css, so the later declaration (shown above) overrides it. D'oh. This file needs a serious cleanup.

The earlier declaration is this one:

Code (index.css - Lines 742 - 747) Select
.dropmenu li a, #top_info > li > a {
padding: 0 7px 0 7px;
display: block;
border: 1px solid transparent;
border-radius: 4px;
}

Antechinus

Lol. Just found a related bit of muppetry. Someone had put a 1px transparent border on the drop menu li's (where you really do not want it, for several reasons) that was then cancelled on hover (yay for that one too).

Code (index.css - Lines 893 - 900) Select
.dropmenu li li {
    margin: 0;
    padding: 0;
    width: 17em;
    font-size: 1em;
    border-radius: 3px;
    border: 1px solid transparent;
}
Code (index.css - Lines 953 - 955) Select
.dropmenu li li:hover {
    border: none;
}

So borders on li's (which are never wanted for presentation) were fighting with borders on anchors (which are wanted for presentation). This is why CSS files should be kept clean and sane, instead of just randomly throwing stuff in and hoping it will all work.

Anywy, this can be fixed too. Hopefully nobody will insist on unfixing it again. :D.

Grammy

It is truly dizzying to watch the CSS Master at work.   :D

(I seriously learn stuff.) 

Antechinus

Quote from: Antechinus on August 26, 2021, 07:18:01 PMAnd a really obvious question: why use the .popup_window class for #main_menu and #genericmenu anyway?

All that does is force you to override all the default styles for that class in index.css (so it won't bork things at > 480px wide) just after they have been defined for actual pop-ups (help, etc). Then you have to redeclare all those styles in responsive.css once width gets down to 480.

It's a tad bonkers. :P If you want a class that is invisible by default, why declare one that is going to screw things? It would make more sense to use a different class for mobile menus, that only has eye candy declared in one place (responsive.css).
Had an idea about this. Have not messed with it on local yet, but I'm pretty sure this could be simplified by using CSS :not() on the initial declarations for the .popup_* classes. So this...

div:not(#main_menu):not(#genericmenu) .popup_window {
    oh-look: lotsa-eye-candy;
}
...should only wallop the standard help pop-ups, etc and leave the menu shenanigans alone. Menu shenanigans would still have to be declared in responsive.css, but at least responsive.css would not have to be overriding index.css, so things should be simpler and more idiot-proof.

Will mess around with it, and see if I can idiot-proof it against myself (always a good starting point). :)

Antechinus

Yay! Found another one. Isn't this fun? :P

.dropmenu, #top_info {
    position: relative;
}

Ok, it works if you are happy with default presentation. Thing is, the drop menus this is meant to deal with are all children of .dropmenu li and #top_info li. Drop menus work just as well if the relative positioning is declared on the li, instead of on the parent ul. This is how they all used to be done.

Problem is that if someone wants to edit the CSS to make the profile/admin/etc menus into a sidebar (which is easy to do, and easy to make responsive) they will be declaring #genericmenu .dropmenu>.subsections>ul as position: static. That then screws up some of the third level menus.

If, on the other hand, the li's with drop menu children had relative position declared on them, they would be fine with any presentation I can think of. So this makes more sense:

#top_info > li, .subsections {
    position: relative;
}

This could be further simplified if, for consistency, the #top_info li's were also given the subsections class:

.subsections {
    position: relative;
}

Which could also simplify this:

.dropmenu .main_icons::before, #profile_menu .main_icons::before, .dropmenu img {
    margin: -3px 8px 0 0;
    vertical-align: middle;
}

Down to this:

.subsections .main_icons::before, .dropmenu img {
    margin: -3px 8px 0 0;
    vertical-align: middle;
}

And while we're on .subsections, there is this:

.dropmenu li li.subsections > a::after {
    position: absolute;
    padding: 5px 0;
    right: 10px;
    font: 83.33%/150% Arial, sans-serif;
    content: "\25ba";
}

The .subsections class is only ever used in SMF for li's that have ul drop menu children. Nothing else. It doesn't need superfluous descendant selectors or tag names, and shouldn't have them. This is all it needs:

.subsections > a::after {
    position: absolute;
    padding: 5px 0;
    right: 10px;
    font: 83.33%/150% Arial, sans-serif;
    content: "\25ba";
}

I'm sure there is oodles more of the RC4 menu code that could be simplified. :)

Advertisement: