Repository navigation
Conversation
Add an optional `action?: React.ReactNode` slot to InternalControlGroup. When provided, the action renders as a sibling of the fused control row via an outer .layout wrapper, so in vertical direction it stays on the side rather than stacking with the controls. The action is a plain slot: it is not passed through flattenChildren, not wrapped in GroupedControlContext, and not counted toward control positions. When action is absent, the controls' markup is unchanged (role=group div remains the root). Adds .layout / .action-slot SCSS (space-xs gap, flex-end alignment), dev permutations exercising an icon Button action in both directions, and unit tests covering action rendering, absence, and position-count isolation. Verification blocked: this workspace has no node_modules or built lib/ artifacts, and the running watch job targets a sibling workspace, so jest (which imports from lib/components) could not be run here.
The .action-slot, .layout-horizontal, and .layout-vertical rulesets held
only Sass '//' line comments. Sass strips those comments and omits the
resulting empty selectors, so no class keys were generated in the CSS
module and styles['action-slot'] evaluated to undefined, making the
action rendering test build the selector '.undefined'.
Use retained CSS block comments ('/* */') as the repository's
selector-only pattern (see src/flashbar/styles.scss) so the selectors
survive generation. Confirmed the keys now appear in the generated
styles.css.js.
Verification:
- quick-build regenerated lib/ (one-shot, no watcher).
- TZ=UTC jest -c jest.unit.config.js src/internal/components/control-group
-> 11/11 pass, including the 4 action tests.
- tsc --noEmit -p tsconfig.json -> clean (no type errors).
Style the action slot so a button fuses with the grouped controls as if it were the last control, in both directions, with per-variant borders: - Icon (borderless) variant: slot supplies the shared gray control border; normal/primary keep their own border, radius + seam only. - Flat inline-start corners, rounded inline-end corners, seam overlap reusing the control negative-margin pattern. Column-gap removed. - Vertical: action spans the stacked column's full height on the inline-end; the column's inline-end corners are squared where it meets the action (scoped to the vertical+action case only). Variant is read defensively from the action element at the ControlGroup level. No control component is touched; the action stays ReactNode-only, is not wrapped in GroupedControlContext, and is not counted as a control. The no-action render path is unchanged. Moves the action permutations to a dedicated action.page.tsx covering icon/normal/primary across both directions; permutations.page.tsx is restored to origin/main. Adds unit tests for the icon vs non-icon border branch.
Polish the three remaining defects on the fused (grouped) action Button; all changes are scoped to `.button.grouped` so standalone buttons are untouched. Only src/button/styles.scss changed (CSS expressed all three, so internal.tsx was not needed). 1. Icon padding: `.button.grouped.variant-icon` uses `$space-field-horizontal` (the controls' horizontal field padding) instead of the stock icon-only `$space-xxs`, so the fused icon cell is not cramped. Three-class specificity beats `.button.variant-icon`. 2. Top border flush / height: `.button.grouped` gets `min-block-size: $size-vertical-input`. The controls floor their height with the same token under box-sizing: border-box (field height == 32px in the default theme). The Button reset is also border-box but had no floor, so with the smaller button vertical padding its box was shorter; under the row's `align-items: flex-end` the shortfall showed as a gap at the TOP. Flooring to $size-vertical-input makes action box height == field height == 32px, flush in both directions. The icon border recolor swaps $border-width-button (1px) -> $border-width-field (1px); both are inside the border-box and equal, so it adds no net height. 3. Text no-wrap inline: `.button.grouped` sets `white-space: nowrap`, overriding the base `.button` `white-space: normal`; non-grouped buttons still wrap. Verification (package root): - npm run quick-build -> OK (one-shot; transient clean ENOTEMPTY once, succeeded on retry). - TZ=UTC node_modules/.bin/jest -c jest.unit.config.js src/button src/internal/components/control-group -> 36 suites, 801 passed, 1 skipped, 0 failed. - npx tsc --noEmit -p tsconfig.json -> clean. - stylelint src/button/styles.scss -> clean; eslint changed files -> clean. - Compiled styles.scoped.css confirms: min-block-size var(--size-vertical-input,32px) + white-space: nowrap on .grouped; padding-inline var(--space-field-horizontal,8px) only under grouped+icon; base .button still white-space: normal.
Addresses the review finding that the prior `min-block-size: $size-vertical-input` floor on `.button.grouped` is arithmetically inert in the default theme: the base Button already resolves to a 32px border-box (line-height 22px + 2x4px block padding + 2x1px border), so flooring to 32px changed no default geometry. Give the fused action the control field's full height recipe instead: add `padding-block: $space-field-vertical` alongside the existing `min-block-size: $size-vertical-input`. The field box is `content + 2*$space-field-vertical + 2*$border-width-field` floored to `$size-vertical-input` under border-box; the button already shares border-box and the same 22px content line-height (via styles-reset font-body-m), so matching the block-padding token and floor makes the button border-box equal the field box by construction, keeping the top borders flush across token modes rather than only where the button and field spacing tokens coincide. normal/primary keep their own border width per spec. The grouped icon border recolor stays inside the border-box, so it adds no net height. Also correct the comments: the base `white-space: normal` comes from styles-reset (not styles.text-wrapping), and drop the inert "shorter box / 2px gap" narrative and restated scope notes. Keeps grouped icon padding ($space-field-horizontal, grouped+icon only) and nowrap on `.button.grouped`; non-grouped buttons unchanged. Verification (default-theme token values): - field: max(32, 22 + 2*4 + 2*1) = 32px - button: 22 + 2*4 + 2*1 = 32px (recipe match) - npm run quick-build -> OK - jest src/button src/internal/components/control-group -> 801 passed, 1 skipped - tsc --noEmit -> clean - stylelint src/button/styles.scss -> clean - eslint src/button/internal.tsx -> clean (unmodified)
Address the review's one blocking finding: the grouped action button's height declarations are default-theme no-ops and the comment overclaimed a universal "equal in every theme / by construction" invariant that the retained $border-width-button for normal/primary breaks. Take the finding's "demonstrate the supported token constraints that make the formulas equal" option and rewrite the comment to match the box model (comment-only; the declarations are already correct and unchanged): - Write out the field recipe max($size-vertical-input, lineHeight + 2*$space-field-vertical + 2*$border-width-field) under border-box. - Show the grouped action shares border-box and the same font-body-m line-height, so adopting $space-field-vertical padding and the $size-vertical-input floor makes the two recipes share every term except the border width. - Scope the EXACT equality to the icon variant, whose border is recolored to $border-width-field, so there every term matches the field token-for-token (the variant the user reported the offset on). The load-bearing fix for that offset is the border recolor giving the icon cell a real 1px edge; padding+floor is the height half. - State normal/primary keep $border-width-button per spec, so their height matches only while the two border tokens coincide (true in the default theme) - the one intentional, documented divergence. Removes the overclaimed universal-invariant wording (the non-blocking comment finding). No declaration changed; non-grouped buttons untouched. Verification: - field: max(32, 22 + 2*4 + 2*1) = 32px - grouped icon action: max(32, 22 + 2*4 + 2*1) = 32px (token-for-token) - npm run quick-build -> OK - jest src/button src/internal/components/control-group -> 801 passed, 1 skipped - tsc --noEmit -> clean - stylelint src/button/styles.scss -> clean - eslint src/button/internal.tsx -> clean (unmodified)
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5119 +/- ##
==========================================
- Coverage 97.70% 97.70% -0.01%
==========================================
Files 990 990
Lines 31818 31828 +10
Branches 11752 11756 +4
==========================================
+ Hits 31089 31098 +9
- Misses 722 723 +1
Partials 7 7 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Related doc:
KaCZyfbvq46DHow has this been tested?
Review checklist
The following items are to be evaluated by the author(s) and the reviewer(s).
Correctness
CONTRIBUTING.md.CONTRIBUTING.md.Security
checkSafeUrlfunction.Testing
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.