[2.x] fix(tags): blank mobile toolbar title when the current tag has no nav item#4821
[2.x] fix(tags): blank mobile toolbar title when the current tag has no nav item#4821karl-bullock wants to merge 1 commit into
Conversation
imorland
left a comment
There was a problem hiding this comment.
Thanks for the thorough writeup — the root-cause analysis here is spot on, and I verified it: SelectDropdown.getButtonContent() derives the label purely from the active child and the nav dropdown passes no defaultLabel, so an unlisted current tag → no active child → blank title. That part's not in question.
Where I'd push back is the layer this is fixed at. Adding the current tag to the nav list doesn't only restore the title — it makes the current tag appear in the index sidebar (desktop and mobile) whenever you're on its page, when it otherwise wouldn't be listed. That's a visible behaviour change to the sidebar beyond the reported bug, and it's the kind of thing that should be a deliberate product decision rather than a side effect of a title fix.
Digging into alternatives, they all felt like they were compensating for the same underlying thing rather than fixing it:
- Passing a
defaultLabelfrom the tags extension — the natural "show the tag name" approach — isn't cleanly reachable: the navSelectDropdownis built inline inIndexSidebar.items()with no hook to supply a per-page label, andextend()can't customise a scalar return, so it'd force anoverride(). - A hardcoded core
defaultLabel("All Discussions") fixes the blank but shows a slightly-wrong title on tag pages, and still leaves the door open on other consumers. - Making the label an
ItemListso it'sextend()-able is really just a single value wearing a list costume — a smell of its own.
The common thread is that they're all working around the title control having nothing to show when no item is active, rather than addressing why it ends up with nothing to show — and the tags list is just what happens to expose it.
Could you take another pass at this? The constraints I'd want a fix to meet: it shouldn't change what appears in the sidebar, it should hold for any consumer that hits the empty-label case (not just tags), and it should avoid an override(). Worth agreeing the approach here before you rebuild it, rather than reworking the current one.
|
Thanks, this is fair. I agree the sidebar behaviour change is the real problem with the current patch: adding the tag to the list to recover the title conflates two separate things. Working within your three constraints (no sidebar change, holds for any consumer, no
So the fix I would propose: 1. Core — defaultLabel={app.current.get('title') ?? app.translator.trans('core.forum.index.all_discussions_link')}That resolves the empty-label case for any consumer, adds nothing to the sidebar, and needs no override. 2. Tags — on a tag page, Net result: an unlisted tag page shows the tag name, the plain index shows "All Discussions", and any other extension caught in the same empty-label spot can set its own page title or fall back to the default. No sidebar change, no The one thing I would want your call on before I rebuild is the |
Fixes #4819
What / why
On phone-width viewports the index toolbar title (the nav
SelectDropdown,App-titleControl) renders blank on tag pages whenever the current tag has no item in the index sidebar nav. In practice that means any secondary tag that is not among the 3 most discussed secondary tags, so the set of affected tags shifts as discussion counts change and the bug looks random from an admin's point of view.Root cause
SelectDropdown.getButtonContent()uses the label of the first active child and falls back toattrs.defaultLabel. The nav dropdown built inIndexSidebar.items()passes nodefaultLabel, andaddTagListonly adds primary tags plus the top 3 secondary tags bydiscussionCount(). When the current tag was excluded by those filters, no child is active and the label collapses to nothing, leaving just the sort caret.Fix
In
addTagList, after the existing filters run, add the current tag to the nav when it did not already get an item.addTagalready marks it active, so the dropdown label, the active highlight, and the desktop sidebar stay consistent, and the current tag is now visible in the nav while you are on its page. Tags that already made the list are unaffected (items.hasguard), and the "More..." link logic is untouched since it only depends on the leftovermorearray.I chose this over passing a
defaultLabelto the nav dropdown in core becauseapp.currentTagbelongs to the tags extension, and the only label core could supply on its own ("All Discussions") would show a wrong title on tag pages instead of a blank one.Testing
Reproduced on a clean 2.x-dev install with 4 secondary tags at descending discussion counts (4/3/2/1), Chromium at 390x844: