feat(remix): add toggle group#71
Conversation
tilucasoli
left a comment
There was a problem hiding this comment.
Review: follow-established-patterns pass
The component is solid — no correctness bugs. The risky mechanics (group→item enabled cascade, roving keyboard focus, and excludeSemantics) are all correctly delegated to naked_ui beta.4, which I verified against the resolved package source. The inline comments are all about matching patterns already established by sibling components; #1 (mergeActiveVariants) and #2 (_withOrientation) are the most substantive.
One finding that couldn't be anchored inline (file not in this diff):
- Missing
FortalToggleGroupsmoke test inpackages/remix/test/components/fortal_widget_test.dart. The sharedgroup('Fortal widgets')block has atestWidgets('renders FortalX')case for every@MixWidgetfortal wrapper (FortalAccordion … FortalToggle … FortalTabBar/Tab/TabView).fortal_toggle_group_styles.dartdefines@MixWidget(name: 'FortalToggleGroup')but norenders FortalToggleGroupcase was added — it's the only fortal wrapper with zero render coverage in the aggregator.
| styleSpec: styleSpec, | ||
| builder: (context, spec) { | ||
| return FlexBox( | ||
| styleSpec: _withOrientation(spec.container), |
There was a problem hiding this comment.
Two sources of truth for flex direction. Direction is already baked into the container at line 108 (FlexBoxStyler(direction: orientation)), then _withOrientation re-patches it onto the already-resolved spec here via a deep container.spec.flex?.copyWith(...) chain that always wins — making the line-108 direction effectively dead. Sibling tabs passes orientation only to NakedTabs and lets the single style spec own visual direction; no post-resolution mutation.
Two costs:
- The deep
copyWithbreaks silently ifFlexBoxSpec/FlexSpecreshape, and a future edit to line 108 has no effect. _withOrientationalso runs on the rawstyleSpecpath, so a caller who supplies a verticalRemixToggleGroupSpecbut leavesorientationat its.horizontaldefault gets it silently forced back to horizontal — defeating the "bypasses fluent resolution" contract documented at lines 89-90.
| StyleSpec<RemixToggleGroupItemSpec> _resolveStyle(BuildContext context) { | ||
| final rawDefault = defaultStyleSpec; | ||
| if (rawDefault != null) { | ||
| final resolved = RemixToggleGroupItemStyler.create( |
There was a problem hiding this comment.
Minor / shared-pattern note (not a regression): on this raw styleSpec branch the group's default item styling comes from an already-resolved spec.item, so its onSelected/onHovered/onDisabled visuals are flattened away — a caller supplying a raw group styleSpec gets a static-looking segmented control. This matches select/menu exactly (inherent to the raw-spec escape hatch), so nothing to fix here — flagging only so the limitation is documented.
Drop the @VisibleForTesting mergeActiveVariants call (and its lint ignore) from RemixToggleGroup. Item defaults are now passed as the raw $item prop and resolved inside each option's WidgetStateProvider, matching RemixSelect/RemixMenu. Context variants targeting items are authored on the item styler instead of the group styler; the composition test is updated to the item-level form, which preserves variant precedence (widget-state beats context). Also add a `## Example` block to the RemixToggleGroup class doc, matching the doc convention used by sibling component widgets. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Every other @MixWidget fortal wrapper has a 'renders FortalX' case in the shared Fortal widgets group; add the missing one for FortalToggleGroup. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Direction was set twice: once on the base container styler and again, post-resolution, by _withOrientation. The post-resolution pass is the one that must run (it is the only point covering the raw styleSpec path, which bypasses styler merging), so the base-styler direction was dead. Remove it and document _withOrientation as the single enforcement point. No behavior change; orientation still drives both layout and the NakedToggleGroup navigation axis on both paths. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Summary
RemixToggleGroup<T>component backed byNakedToggleGroup<T>Why
Remix had standalone toggles but no single-select segmented-control component. Toggle Group provides one Tab stop, focus-only arrow navigation, explicit Space/Enter activation, orientation-aware movement, Home/End, disabled-item skipping, optional looping, and RTL-aware horizontal navigation.
Dependency note
The publishable dependency remains
naked_ui: ^1.0.0-beta.3. A temporary dependency override tracksnaked_uimain atd414875, where the roving-focus Toggle Group was merged in naked_ui PR #68. The override can return to a hosted version after the next naked_ui release.Verification
fvm dart run melos run ci --no-selectgit diff --check origin/main...HEAD