AO3-5792 Add missing class to additional tag groups in Tag Sets to fix skins

[slavalamp]

Pull Request Checklist

• Have you read "How to write the perfect pull request"?
• Have you read the contributing guidelines?
• Have you added tests for any changed functionality?
• Have you added the Jira issue number
  as the first thing in your pull request title (e.g. AO3-1234 Fix thing)
• Do you have fewer than 5 pull requests already open? If not, please wait
  until they are reviewed and merged before creating new pull requests.

Issue

https://otwarchive.atlassian.net/browse/AO3-5792

The issue only mentions Reversi but Happy 17th! and Low Vision Default also have the wrong border color there.

Purpose

Adds a missing "group" class to additional tag groups in Tag Sets so that they're targeted by Reversi, Happy 17th! and Low Vision Default site skins correctly.

This does not affect the default skin or most other public skins. Out of the three that are affected: Fixie (911) seems to get fixed by this as well, The Hustings (889) also becomes consistent (can't say "fixed" because the contrast there is terrible), and Panda Madness (890) doesn't account for these tag groups and has them look a bit out of place either way.

Testing Instructions

1. Create a tag set with 31+ Additional tags (the number in the Jira issue is incorrect)
2. Switch skin to Reversi, or use ?site_skin=929 switch
3. The tag group headers in the Additional tags section should have dark backgrounds

References

n/a

Credit

slavalamp (they/them)

[sarken]

Thanks for working on this, and for noting the issue with the number of tags mentioned in the Jira issue! I've updated the issue with the new information.

[sarken]

This is a good start! Can you make group come after listbox, though? We usually have the most general classes on the right and most specific on the left.

This is also missing a fix for the second part of the issue:

The "Expand All"/"Contract All" buttons are oversized compared to other buttons on the page

The issue sort of assumes these problems are with the Reversi skin, but as you've discovered, the problems are with the markup. The troublesome buttons, for example, are in a heading that doesn't have the same classes as the rest of the headings on this page. The simple fix would be to add the expander_parent heading classes to the <h3> on line 14 and call it a day:

otwarchive/app/views/owned_tag_sets/_show_tags_alpha_listbox.html.erb

Lines 13 to 17 in 6443729

	<li class="<%= tag_type_css_class(tag_type) %> alphabet tagset index group listbox">
	<h3>
	<%= ts("%{tag_type}", tag_type: tag_type_label_name(tag_type).pluralize) %>
	<%= expand_contract_all %>
	</h3>

That's probably the best place for you to stop for this pull request, but since you've submitted several front end pull requests, I'll walk you through some of the other issues I found with this code just to help you get a little more familiar with our front end conventions. (To avoid scope creep on the original issue, I'll make a new issue and submit a pull request for these fixes and more.)

The problem with the simple solution is expander_parent is a class name that doesn't fit with our naming system, so simply copying it is just exacerbating the problems with this page.

Likewise, if we happen to glance up a line to 13, we'll find that the classes on that <li> are wrong. Under our system, nothing should ever have both the index and listbox classes:

• the rules for index tell us it's reserved for lists: <ul>, <ol>, and <dl>
• the rules for listbox say it can be used on many elements, but not an element with restricted children -- so never lists

The tagset class on line 13 is also not used very well. (And, yeah, it breaks our naming rules. 😩 But this is one we have to live with.) If you put this CSS in your sandbox.css file and load a tag set, you can probably see why: .tagset.listbox { background: palegoldenrod; !important; }

What makes the "Additional Tags" listbox a "tagset" but nothing else? It's just one of several types of tag within a tag set. A general tag like tagset should either be on all of the listboxes or -- even better -- on the ol.index that contains all of these listboxes.

However, it turns out there are two indexes for the listboxes on the page. It is most likely because someone wanted to add that <h4> with the note about unassociated fandoms and characters, and an <h4> can't be a direct child of a list. So the thing to do is move that <h4> inside the relevant listbox -- it doesn't make sense to have it above the <h3> it's describing, anyway -- and combine the two lists into one.

Another issue is the buttons for expanding, contracting, and shuffling. They're clearly misaligned, and that's because the "Expand All"/"Contract All" buttons are inside a <span class="actions"> while the arrow buttons for expanding, contracting, and shuffling have the action class applied directly to the link. Ideally, they should all be inside one <span class="actions"> to keep things in the markup tidy and consistent.

[slavalamp]

i've made the changes, thanks for the explanations!

i was so excited to have figured out the missing group class thing on my own that i completely overlooked all the button problems...

[sarken]

Looks great now -- thank you!