News:

SMF 2.1.4 has been released! Take it for a spin! Read more.

Main Menu

2.1 - Easy CSS crud reduction, if anyone can be bothered.

Started by Antechinus, September 27, 2019, 09:30:53 PM

Previous topic - Next topic

Antechinus

Was just looking around 2.1 a bit, and noticed something in the CSS.

/* Let's get a bit more flexibility in font sizes for quotes and code. */
/* We just need to stop em compounding when elements are nested. */
.bbc_standard_quote .bbc_alternate_quote, .bbc_alternate_quote .bbc_standard_quote,
.bbc_standard_quote .bbc_code, .bbc_alternate_quote .bbc_code, .bbc_standard_quote .codeheader,
.bbc_alternate_quote .codeheader, .bbc_standard_quote .quoteheader, .bbc_alternate_quote .quoteheader {
    font-size: 1em;
}

It's pretty obvious that whoever was messing around with the coding for bbc quotes and code does not actually know what this block of code was for. How can I tell? :D

Well the code for blockquotes is now this:

/* Blockquote stylings */
blockquote {
    margin: 0 0 8px 0;
    padding: 6px 10px;
    font-size: small;
    border: 1px solid #d6dfe2;
    border-left: 2px solid #aaa;
    border-right: 2px solid #aaa;
}

Font-size: small; is an absolute sizing that will not compound when nested. That's fine if you want that font-size. No problem there. However, the code blocks use font-size: 0.85 em; instead, and of course that is a sizing that will compound when nested.

The whole point of the block of code quoted at the beginning of the post (ie: Line 384+ in whatever RC2+ nightly is on my box) is to stop font-sizing compounding in nested quotes. It even says so. Furthermore, since it uses classes and descendants for targetting it actually overrides your declared font-size for quotes, because that is only declared on the basic tag name.

If you want to use an absolute sizing for quotes then it would make sense to do the same for code (but please, not x-small as people do want to read them). If you are going to do that, then there's no need for additional code to stop nested quotes and code compounding for font-size. The main reason it was put there originally was because at the time IE8 was relevant, and IE8 did not support font sizing in rem. If you want maximum flexibility of font sizing, without any worries about what will happen when elements are nested, rem is the way to go. It's supported by just about anything that is actually in use these days (see https://caniuse.com/#feat=rem if you're worried).

So, suggestion. Ditch this pile of crud:

/* Let's get a bit more flexibility in font sizes for quotes and code. */
/* We just need to stop em compounding when elements are nested. */
.bbc_standard_quote .bbc_alternate_quote, .bbc_alternate_quote .bbc_standard_quote,
.bbc_standard_quote .bbc_code, .bbc_alternate_quote .bbc_code, .bbc_standard_quote .codeheader,
.bbc_alternate_quote .codeheader, .bbc_standard_quote .quoteheader, .bbc_alternate_quote .quoteheader {
    font-size: 1em;
}

And use this for blockquote and .bbc_code:

    font-size: 0.85rem;
That way they're equally readable and match well, and there are no problems with sizes compounding, and you save a pile of crud in your file. :)

(This tip is provided free of charge because someone might want cleaner code. Ant is too jaded to bother doing the whole GitHub thang and the whole arguing with people about what gets done and what doesn't thang.)

ETA: Just noticed something else:

blockquote cite {
    display: block;
    border-bottom: 1px solid rgba(0, 0, 0, 0.1);
    font-size: 0.9em;
    margin-bottom: 3px;
}

There's no need to call rgba transparency there. It's just bloat. This would be just as good for looks and is less verbose:

    border-bottom: 1px solid #ccc;
I do hope someone has not been making a habit of using full rgba syntax where a basic hex code will do just as well.

Antechinus

Ok, thanks for the laughs. I just found one of the funniest things I've ever seen, and damned near fell off the chair laughing. :D

.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;


Notice those last two lines. Now tell me they're not hilarious. No sh!t, this is truly awesome stuff. I'm still laughing. :D

Arantor

The min-height has some use. The max-height is... not going to help.

Gwenwyfar

You're gonna be busy if you start reporting those. Maybe easier to make some PRs.
"It is impossible to communicate with one that does not wish to communicate"

Antechinus

Quote from: Arantor on September 28, 2019, 12:19:53 PM
The min-height has some use. The max-height is... not going to help.

The bit that had me ROFLMAO was some earnest young munchkin was obviously struggling with this conundrum, wondering why their min-height wouldn't work, and finally had the bright idea of thumping it with !important. Hey, it works now! Yay me!

Quote from: Gwenwyfar on September 28, 2019, 12:28:50 PM
You're gonna be busy if you start reporting those. Maybe easier to make some PRs.

Maybe. Maybe not. Frankly the easiest thing to do would be to rewrite the whole file and just hand over a copy. That way I'd have a good one even if nobody could be bothered implanting the changes in the repo. I won't be doing that this week though as I'm in the middle of something else. I just took a quick look at 2.1 as a bit of an amusing sidetrack.

But if you want to do PR's here's a good one for you: the quickbuttons code is rooted. No keyboard accessibility for most of it. Why not? No href's on the anchors, so there's no way of tabbing to them. This is a very easy fix for the "More.." button. You just change this:

// Handle the sublist
if($key == 'more')
{
$output .= '
<li class="post_options">' . $txt['post_options'] . '


To this:

// Handle the sublist
if($key == 'more')
{
$output .= '
<li class="post_options"><a href="#" onclick="event.preventDefault()">' . $txt['post_options'] . '</a>


Straight away you have full keyboard access to the drop menu. Easy. Piece of cake, and y'know what? Back when it was originally coded, it did have full keyboard access to the drop menu.

So that's the "More.." button sorted. The next problem is the Quick Edit button. That has no href by default either, and here's the fun part. Code is this:

else
$html .= '
<a' . (!empty($li['href']) ? ' href="'.$li['href'].'"' : '') . (!empty($li['javascript']) ? $li['javascript'] : '') . '>
' . (!empty($li['icon']) ? '<span class="main_icons '.$li['icon'].'"></span>' : '') . (!empty($li['label']) ? $li['label'] : '') . '
</a>';


So just as a test I threw in the same href as I used to fix the "More.." button behaviour.

else
$html .= '
<a' . (!empty($li['href']) ? ' href="'.$li['href'].'"' : 'href="#" onclick="event.preventDefault()"') . (!empty($li['javascript']) ? $li['javascript'] : '') . '>
' . (!empty($li['icon']) ? '<span class="main_icons '.$li['icon'].'"></span>' : '') . (!empty($li['label']) ? $li['label'] : '') . '
</a>';


Which should work, but it doesn't. What it does is bork the markup. It puts the correct code in the anchor opening tag, but for some weird reason also outputs the anchor closing tag as this:

</ahref="#">

Which is highly inventive, but unfortunately completely screws any semblance of functionality. It's also not at all clear why it is doing this, since there's nothing in the template to indicate it would behave like that. So if you want a juicy bug to get your teeth into, that would be a good one to start with. If you can fix that one, suddenly you'll have keyboard accessibility for the quick edit too (and probably several other things).

Gwenwyfar

QuoteMaybe. Maybe not. Frankly the easiest thing to do would be to rewrite the whole file and just hand over a copy.
Ditto.

QuoteBut if you want to do PR's here's a good one for you: the quickbuttons code is rooted. No keyboard accessibility for most of it. Why not? No href's on the anchors, so there's no way of tabbing to them. This is a very easy fix for the "More.." button. You just change this:
I'm assuming that "you" wasn't me "you". I'm done with the duct taping.

Wouldn't it make more sense to add a <button>? Since it's an empty anchor, that's more like button territory.
"It is impossible to communicate with one that does not wish to communicate"

SychO

Nothing is stopping you from submitting changes to the repo you know..
Checkout My Themes:
-

Potato  •  Ackerman  •  SunRise  •  NightBreeze

Bloc

Isn't 2.1 in RC2/RC3 phase? Rewriting lots of template code prob. won't be added into main repo until after a stable version is out. Which is anyone's guess..so submitting things that will go in a 2.2 or even 3 version seems kinda ..well, you have to be patient. :) 

Although default theme can always be fixed/improved/polished its "done" now, and said efforts might possibly be better spent in doing custom themes....until that time when default theme CAN catch up. Wrong?

Gwenwyfar

Quote from: Bloc on September 28, 2019, 03:10:42 PM
Isn't 2.1 in RC2/RC3 phase? Rewriting lots of template code prob. won't be added into main repo until after a stable version is out. Which is anyone's guess..so submitting things that will go in a 2.2 or even 3 version seems kinda ..well, you have to be patient. :) 

Although default theme can always be fixed/improved/polished its "done" now, and said efforts might possibly be better spent in doing custom themes....until that time when default theme CAN catch up. Wrong?
Even before RC any big changes were frowned against, even just for the theme. 2.1 had all sorts of ideas from different people. It was all very debatable.

The theme is "done" but still full of issues. Which at least there is someone working on, now, but was largely abandoned before. It would be a big sham if it's released this way because then every single theme made on it will have to be fixed, instead of it being fixed only once in the main repo. If this was done, everyone could work on a proper thing that is easier to handle.
"It is impossible to communicate with one that does not wish to communicate"

Bloc

Agree on that. In the end its up to the team really. It would seem theres alot of issues to deal with so the changes would not be minimal.

Antechinus

Bloc: the issue I suggested is an actual bug, at least IMO. It's also one that (as shown) can be fixed simply by making the offending elements more consistent with the other button in the same strip of buttons. IOW, give them all a href. That's all it takes. They should already be like that, since that's how they started out. What's happened is that someone has written new global code to generate all quickbuttons, which is fine as a concept, but has never give any thought to how anyone other than themselves might want to use the interface, which is frankly pretty slack.

And CSS bugs and bloat in an RC are obvious candidates for fixes, since they don't require any changes in markup or logic.

Gwenwyfar

And not the only one, by far. If you add "general bloat and problems that can be fixed in the css only" that list is quite hefty. Although I haven't been keeping track of what is being fixed since I left the team. I'm hoping the list will be smaller by release.
"It is impossible to communicate with one that does not wish to communicate"

Antechinus

My gut feeling, based on my recent messing around with a similar situation in 2.0.x default CSS, is that the final version of 2.1 index.css could be shipped at around 60kB uncompressed, with no loss of functionality or looks. In fact it could even look a bit better in some details, while still being the same theme, and would be easier to customise.

From a fairly quick scan and a little bit of messing around, the situation appears to be similar in several other CSS files too.

Gwenwyfar

"It is impossible to communicate with one that does not wish to communicate"

SychO

Quote from: Antechinus on September 28, 2019, 06:46:46 PM
Bloc: the issue I suggested is an actual bug, at least IMO. It's also one that (as shown) can be fixed simply by making the offending elements more consistent with the other button in the same strip of buttons. IOW, give them all a href. That's all it takes. They should already be like that, since that's how they started out. What's happened is that someone has written new global code to generate all quickbuttons, which is fine as a concept..

The code that you are referring to was not there and has not been there for many years, I am the one who wrote the generic quickbuttons template and all I did not remove important relevant code whatsoever, fact is, the issues you mention were there before the generic quickbuttons template was made and are still present now.

Quote from: Antechinus on September 28, 2019, 06:46:46 PM
but has never give any thought to how anyone other than themselves might want to use the interface, which is frankly pretty slack.
So your statement is irrelevant.
Checkout My Themes:
-

Potato  •  Ackerman  •  SunRise  •  NightBreeze

Antechinus

One thing that would really help is getting rid of floats as much as is (easily) possible. If things like buttonlists are done with either inline-block or table-cell or flex, then suddenly there's no need to worry about clearing floats or changing float direction for RTL. Inline-block is probably the simplest for a lot of cases since it doesn't require any declarations on the parent. It wasn't an option when 2.0.x was coded, so some of the thinking that went into 2.1 alpha was affected by precedent, and floats got used because everyone knew how they worked.

Antechinus

Quote from: SychO on September 28, 2019, 07:10:22 PMThe code that you are referring to was not there and has not been there for many years, I am the one who wrote the generic quickbuttons template and all I did not remove important relevant code whatsoever, fact is, the issues you mention were there before the generic quickbuttons template was made and are still present now.

In that case, someone removed the href before you got to it. I know for a fact it was there originally. But ok, if you're cool with thinking about how other people might want (or need) to use the interface, then surely adding a href is the sane option here. There's no good argument for not having it, AFAICT. Why make special cases of those two buttons just to reduce functionality?

SychO

No one made a special case of those two buttons, it's just that no one thought of what you are mentioning.
Checkout My Themes:
-

Potato  •  Ackerman  •  SunRise  •  NightBreeze

Antechinus

I thought of it, back when I wrote the alpha code. ;) I mentioned that before. They were all marked up the same, except that at the time the quick modify was still using the old separate image as well. If someone fluffed it after that, it ain't on me. And fair enough, if someone fluffed it before you got to it, that particular change isn't on you either.

Anyway, I think it would make sense to have a default href="#" so that all buttons in that class are accessible by keyboard. Offhand I can't think of any cases where that would break functionality, as long as they have the onclick="event.preventDefault()" to go with the href.

It's just weird that the current code for generating quickbuttons screws the anchor's closing tag when you add the href and onclick in what seems like the obvious place in the ternary. That's definitely unusual behaviour. If that bug is fixed, it should all be good to go (and no, I don't know what's wrong with it at the moment, but I suspect it's somehow tied to javascript).

SychO

Well, either way it doesn't matter who did what when, I am sure they were trying to do their best, the only thing that matters now is that this was noticed and can be fixed.

It would of course be more of help if you were to post a PR where you submit all your suggested changes since it seems like you are reviewing all of the CSS code, but thank you for the reports nonetheless :)

I posted a fix for a couple of issues, I didn't for the .popup_window element's min&max height because I have plans for that family of elements(https://github.com/SimpleMachines/SMF2.1/issues/5665)
PR: https://github.com/SimpleMachines/SMF2.1/pull/5812
Checkout My Themes:
-

Potato  •  Ackerman  •  SunRise  •  NightBreeze

Advertisement: