css: Use SCSS nesting in reactions.scss.

[vrongmeal]

No description provided.

[vrongmeal]

Hey @timabbott. I broke this PR into multiple small commits so that it is easy for you to review. Still a WIP (the part inside .message_reactions seems a little tricky). If you could check this out once that would be great. Thanks!

[vrongmeal]

@timabbott This is ready for review.

[timabbott]

@vrongmeal cool! I was thinking while reviewing this that it might be worth our adding a bit of tooling around this migration. In theory, we should be able to check for commits that aren't supposed to change the output whether indeed that's their effect using a tool (e.g. run the preprocessor and then diff the output). Might be worth doing that to simplify the process here.

[timabbott]

Looks great, merged, thanks @vrongmeal! I squashed a few of the commits together into a20b485; I think that's just as easy to review as having those be separate. I think while we're working on this CSS file, we should:

• Look at the other rules in reactions.scss that are not nested inside something and figure out if they should be -- e.g. emoji-popover-emoji should probably be inside emoji-popover. And maybe .message_reaction .emoji should be inside .message_reactions too?

The overall goal here should be something that's easy to read and understand, and I don't think we're there yet.

[timabbott]

I should say that overall this was very pleasant to review, and I think this approach could be successful for future refactors. I think you can proceed to do these files next with the same approach:

• app_components.scss, components.scss (mostly new-style)
• static/styles/hotspots.scss, static/styles/alerts.scss, and static/styles/lightbox.scss

(I'm trying to pick ones that seem quick and I know don't have active PRs).

[vrongmeal]

I was thinking while reviewing this that it might be worth our adding a bit of tooling around this migration. In theory, we should be able to check for commits that aren't supposed to change the output whether indeed that's their effect using a tool (e.g. run the preprocessor and then diff the output). Might be worth doing that to simplify the process here.

This definitely would be tricky but I think it's worth to do some homework and see if we can come up with something. Would definitely make these refactors easier

[vrongmeal]

Look at the other rules in reactions.scss that are not nested inside something and figure out if they should be -- e.g. emoji-popover-emoji should probably be inside emoji-popover. And maybe .message_reaction .emoji should be inside .message_reactions too?

I was actually thinking just the opposite. Nesting in CSS is expensive and I think if we can reach a state where there is less nesting and more classes, that's just as readable and actually better IMO. It's going to be a little hard but I think it's worth looking once if we could actually eliminate some nesting.

[timabbott]

Yeah, I guess I have a few thoughts here:

• Readability and understandability of CSS is really important, and barring actual evidence of a performance issue, usually much more important than the performance impact of things like this. So, for example, I think it's really good that our settings CSS largely lives inside a #settings_overlay top-level block -- that makes it really clear that you don't need to look at that CSS to figure out whether it applies to other parts of the site. I think a well-architected CSS setup would involve having a small set of standard components shared by multiple components (e.g. buttons, toggles, etc.) that are in common CSS files like app_components.scss, and then most CSS specific to a single component living inside nesting for that component.
• I'm not convinced CSS nesting is actually expensive in cases like this, where basically we're nesting CSS logic to only run inside a particular component. So for example, I think the emoji-picker-specific CSS should live inside a top-level emoji picker block.
• The place where removing CSS nesting can be really valuable is if we want to reuse CSS. So, for example, I could imagine moving the top-level .message_reaction CSS block outside .message_reactions so that we can easily reuse that CSS class in documentation pages should we in the future want to do that. The .emoji_alt_code is an example of that, since we show it both in the settings UI for selecting it as well as in the actual main Zulip UI when it's selected.

Does that reasoning make sense?

This definitely would be tricky but I think it's worth to do some homework and see if we can come up with something. Would definitely make these refactors easier

Yeah, and also easier to review; if half of your commits can say "Verified no changes to CSS output" in the commit message, I could be a lot faster in reviewing those. A possible tool structure could be this:

• Run the CSS preprocessor on the file, outputting it to a file.
• Make the change
• Run the CSS preprocessor on the file, outputting it to another file.
• Depending on what happens with ordering, either just diff the outputs or use e.g. the diff algorithm in modern git with colorMoved enabled to see that the only things that changed was reordering of content (the git version would involve a special repo that just gets automated commits from the tooling).

[vrongmeal]

Does that reasoning make sense?

Yeah it does. It's something I read some time back about how the nesting works. So when we write #settings button and then apply some CSS rules, what the browser actually does is it selects all the buttons and then moves up and searches which among them belong to #setting. Nesting is expensive this way. I do agree with your points though.