Repository navigation
Render language tab sets as a dropdown - #4198
alexmarhaba wants to merge 8 commits into
Conversation
Tab sets in the `languages` group carry one option per client language, which wraps onto two rows as a horizontal strip and pushes the code sample down the page. Those now render as a <select> instead. The gate is the group key: `languages` defaults to a dropdown, every other group is unchanged. Authors can force either rendering on a specific tab set with `:dropdown: true|false`. The tab strip stays in the markup and tabs.ts adds .tabs-dropdown-active at runtime to hide it, so pages without JavaScript keep working tabs rather than a dead control, and there is no flash of every language on load. The <select> only ticks the radio input the tab label would have ticked, so grouping, sync, sessionStorage and the ?languages= URL parameter all keep working untouched — a dropdown and a tab strip in the same group stay in sync with each other. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Docs preview (local build)Handbook preview: https://docs-v3-preview.elastic.dev/elastic/docs-builder/pull/4198/ Sample changed pages: |
eslint's no-unexpected-multiline flagged the indexed access split across lines. Use a local instead. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Requesting changes: dropdown mode currently leaves hidden fallback radios keyboard-interactive, which can create invisible focus targets and state drift from the visible select control.
What is this? | From workflow: PR Review
Give us feedback! React with 🚀 if perfect, 👍 if helpful, 👎 if not.
| if (wrapper) { | ||
| wrapper.hidden = false | ||
| } | ||
| tabs.classList.add('tabs-dropdown-active') |
There was a problem hiding this comment.
[MEDIUM] Dropdown mode leaves hidden radios interactive and unsynced
Once tabs-dropdown-active is applied, the tab labels are hidden but the fallback radio inputs remain keyboard-focusable (tabindex="0" in TabItemView.cshtml). A keyboard user can tab to those now-invisible radios and change selection via arrow keys; that flips visible content through CSS without going through the <select> change path, so dropdown value/group sync/session state can drift.
Please make radios non-interactive when dropdown mode is activated (for example in initDropdowns(), set each .tabs-input to tabIndex = -1 and/or disabled) so the <select> is the only active control while JS is on.
There was a problem hiding this comment.
Good catch — this was real. Fixed in cb26271.
initDropdowns() now sets tabIndex = -1 and aria-hidden="true" on the tab set's own .tabs-input elements when the dropdown takes over, so the <select> is the only control in reach while JS is on. I scoped it with :scope > .tabs-input so a nested tab set keeps its own inputs.
I went with tabIndex = -1 over disabled because the radios still have to be settable from JS and still drive the :checked CSS that shows the panels — this only removes them from the keyboard path, it does not change how the panels work.
Tab sets that stay as a strip are untouched, and there are two new tests covering both sides: radios go to -1 in dropdown mode, and stay at 0 when the strip is still visible.
The radios that drive the panels are opacity-0 rather than removed, and TabItemView gives them tabindex="0". With the tab strip hidden, a keyboard user could tab onto an invisible radio and arrow through it, flipping the visible panel via CSS without the <select> ever seeing a change event — leaving the control, the group sync and sessionStorage pointing at a different tab than the one on screen. Set tabIndex = -1 and aria-hidden on them when the dropdown takes over, so the <select> is the only control in reach. Tab sets that stay as a strip are untouched. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Approved.
What is this? | From workflow: PR Review
Give us feedback! React with 🚀 if perfect, 👍 if helpful, 👎 if not.
There was a problem hiding this comment.
Requesting changes: the newly added tab directive tests currently fail to compile due to unresolved test-framework symbols and must be aligned with the project’s test setup before merge.
What is this? | From workflow: PR Review
Give us feedback! React with 🚀 if perfect, 👍 if helpful, 👎 if not.
| } | ||
| } | ||
|
|
||
| public class LanguagesGroupRendersAsDropdownTests(ITestOutputHelper output) : DirectiveTest<TabSetBlock>( |
There was a problem hiding this comment.
[HIGH] New tests introduce unresolved xUnit symbols in a non-xUnit test project
This file currently compiles with the existing test stack, but the new cases add ITestOutputHelper and [Fact] without adding matching references. On this branch, dotnet test tests/Elastic.Markdown.Tests/Elastic.Markdown.Tests.csproj -v minimal --nologo fails with CS0246 for both ITestOutputHelper and FactAttribute at these new lines, so CI cannot pass as-is.
Please rewrite these new test classes to use the same framework pattern already used elsewhere in this file (or add the required dependencies consistently), so the project builds again.
There was a problem hiding this comment.
Correct, and fixed in b621711.
Root cause was the branch aging: #4182 migrated this project from xUnit v3 to TUnit after these tests were written. The four dropdown-gate classes still used the old pattern, so once main was merged in they stopped compiling.
They now match the classes around them — [InheritsTests] on the class, parameterless primary constructor, [Test] per case.
Verified locally with the same entry point CI uses, dotnet run --project build -c release -- unit-test: full suite green, and all four classes appear in the Elastic.Markdown.Tests report.
main migrated this project from xUnit v3 to TUnit (#4182) while this branch was open. The four dropdown-gate classes still declared an ITestOutputHelper parameter and [Fact], neither of which resolves any more, so the merged branch failed to compile with CS0246. Match the classes around them: [InheritsTests] on the class, a parameterless primary constructor, and [Test] on each case. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Requesting changes.
What is this? | From workflow: PR Review
Give us feedback! React with 🚀 if perfect, 👍 if helpful, 👎 if not.
| * fallback; tabs.ts adds .tabs-dropdown-active once the <select> is live. | ||
| */ | ||
| .tabs.tabs-dropdown-active { | ||
| .tabs-label { |
There was a problem hiding this comment.
[MEDIUM] Dropdown-active styles leak into nested tab sets
The new .tabs.tabs-dropdown-active rules apply to all descendant .tabs-label and .tabs-content nodes, not just this tab set's own items. If a tab panel contains a nested tab set, its labels are hidden and its content styling is rewritten unexpectedly.
Please scope these selectors to direct children (for example > .tabs-label and > .tabs-content) so nested tab sets keep their own behavior.
There was a problem hiding this comment.
Confirmed and fixed in 127352b. Both rules now use > .tabs-label / > .tabs-content.
Worth spelling out why this one bites harder than it looks: a nested tab set has no <select> of its own, so hiding its labels left it with no control at all — not just restyled, unusable.
Verified on the compiled output rather than by eye, since jsdom does not apply CSS. _static/styles.css now emits:
.tabs.tabs-dropdown-active>.tabs-label
.tabs.tabs-dropdown-active>.tabs-content
Note the pre-existing .tabs-input:checked + .tabs-label + .tabs-content rule in the base block is still descendant-scoped. I left it alone as out of scope here — the sibling combinators limit the blast radius — but it is the same class of issue if anyone wants it tightened separately.
| // The checked input is the source of truth — it may have been restored | ||
| // from sessionStorage or a URL param by ready(). | ||
| const checked = tabs.querySelector<HTMLInputElement>( | ||
| '.tabs-input:checked' |
There was a problem hiding this comment.
[MEDIUM] Initial checked-radio lookup can read nested tab state
This lookup is unscoped:
const checked = tabs.querySelector<HTMLInputElement>('.tabs-input:checked')In a dropdown tab set that contains nested tabs, if a non-first outer tab is restored/selected, traversal can encounter a checked nested radio before the outer set's checked radio and initialize the <select> to the wrong ID (or no matching option).
Please scope this to the current tab set's own radios (e.g. :scope > .tabs-input:checked).
There was a problem hiding this comment.
Real, and reproduced before fixing. Fixed in 127352b with :scope > .tabs-input:checked.
I wrote the failing case first to be sure it was not theoretical: outer tab 2 restored as checked, outer tab 1 holding a nested tab set whose own first radio is checked. The nested radio comes first in document order, so select.value was being set to an id matching none of its options. Test failed on the old code, passes now.
This also makes the file consistent — the tabIndex = -1 fix in cb26271 was already using :scope >, and this lookup was the one place still walking the whole subtree.
|
The overlap is real, and this PR can land without extracting a shared component. The API Explorer language control ( Unifying the markup means replacing either the native What should stay aligned is the look. API pages already have a language control, and a second style beside it will read as two products. I'll look at both and follow up on what this dropdown should match. |
Both the CSS and the initial select lookup reached into nested tab sets:
- `.tabs.tabs-dropdown-active { .tabs-label { ... } }` compiles to a
descendant selector, so a nested tab set inside a panel had its labels
hidden and its content padding rewritten. With no <select> of its own,
that left it with no control at all.
- `tabs.querySelector('.tabs-input:checked')` walks the whole subtree. If
a non-first outer tab is the restored one, the nested set's checked
radio comes first in document order, so the <select> initialised from
an id matching none of its options.
Use direct children in both places, matching the scoping already used
for the tabindex fix. Two tests cover the lookup and the nested set
keeping its own radios; the compiled CSS now emits `>` combinators.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Requesting changes.
What is this? | From workflow: PR Review
Give us feedback! React with 🚀 if perfect, 👍 if helpful, 👎 if not.
| function syncSelectToLabel(label: HTMLElement) { | ||
| const select = label | ||
| .closest('.tabs') | ||
| ?.querySelector<HTMLSelectElement>('.tabs-select') |
There was a problem hiding this comment.
[MEDIUM] Descendant .tabs-select lookup can desync nested dropdowns
syncSelectToLabel() currently does label.closest('.tabs')?.querySelector('.tabs-select'), which searches the entire descendant tree. For a tab strip that has no own dropdown but contains a nested dropdown tab-set inside one panel, this picks the nested <select> and assigns it the outer label's for id.
That id does not belong to the nested options, so the nested dropdown can be set to an invalid/empty value while its radios remain on a different selection. Please scope this to the current tab-set's own toolbar (for example :scope > .tabs-select-wrapper > .tabs-select) so only the owning tab-set is updated.
tabs.ts: main moved tab init to delegated change/beforematch listeners and hid inactive panels with hidden="until-found". The dropdown code is kept on top of that. The select handler now syncs the set's panels, because setting a radio from script fires no change event, and a find-in-page match also moves the select to the revealed tab. tabs.test.ts: both sides added the file. Both suites are kept, with the dropdown helper renamed to dropdownTabSet to avoid a name clash. TabSetViewModel: main added a design-system tab-set path that builds the view model without the new dropdown members, so they are no longer required and default to the plain tab strip. That path renders no dropdown yet. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
![infra-vault-gh-plugin-prod[bot]](https://avatars.githubusercontent.com/u/6764390?s=60&v=4){kind=small}
| > .tabs-content { | ||
| @apply border-t-0 pt-4; | ||
| } |
There was a problem hiding this comment.
Severity: P3 (Low)
Keep inactive panels at zero padding. This rule has the same specificity as .tabs .tabs-content[hidden='until-found'] and appears later, so pt-4 overrides its padding: 0. In browsers supporting hidden="until-found", every inactive language panel therefore retains a padding box, adding blank space around the selected sample that grows with the number of languages. Apply the padding only to the visible panel or restore zero padding for hidden panels afterward.
Generated by Libra. React with 👍 or 👎 to give feedback on this comment.
| [Test] | ||
| public void DefaultsToDropdownForLanguages() => Block!.RenderAsDropdown().Should().BeTrue(); |
There was a problem hiding this comment.
Severity: P3 (Low)
Add assertions against the rendered dropdown HTML, not just RenderAsDropdown(). All four new C# tests check only the gate, while the TypeScript tests supply handwritten markup. Nothing verifies that the actual renderer emits the dropdown, that option values reference the generated radio IDs, or that opting out removes the dropdown, so a broken server-to-client contract would still pass these tests.
Generated by Libra. React with 👍 or 👎 to give feedback on this comment.
Summary
Tab sets in the
languagesgroup carry one option per client language. On a page like the vector and full-text search quickstart that's nine tabs, which wrap onto two rows and push the code sample down. This renders those as a<select>instead.Nothing else changes:
api-examples(49 uses),docker(25),kibana-api(16), and the two-optionos/operating-systemspickers all stay as tab strips — a dropdown would be a downgrade for those.Current
Proposal
How it works
The
<select>doesn't render anything itself. It ticks the same hidden radio input the tab label would have ticked, and the existing CSS shows the matching panel. So grouping, cross-page sync,sessionStorage, and the?languages=pythonURL parameter keep working without being touched — a dropdown and a tab strip in the same group stay in sync with each other.The tab strip stays in the markup;
tabs.tsadds.tabs-dropdown-activeat runtime to hide it. Without JavaScript you get working tabs rather than a dead control, and there's no flash of every language on load.The gate is the group key, with a per-tab-set override:
::::{tab-set} :dropdown: true # or :dropdown: false to force a tab stripTesting
Assets/tabs.test.ts— 6 jsdom tests, including dropdown ↔ tab-strip sync in both directions and restore-from-session.tsc, eslint, and prettier clean.languagestab sets became dropdowns, defaulted to Python, and stayed in sync; theoperating-systemstabs on the same page stayed tabs.Open questions for reviewers
<select>(_ApiCodeSample.cshtml+api-docs.ts), which even shares thetab-id-storage namespace. I wrote parallel CSS rather than extracting a shared component, so the site would have two similar-but-not-identical pickers. Happy to unify first if that's preferred.languagestoday.api-examplesis the obvious next candidate — deliberately left out.🤖 Generated with Claude Code