[Form Kit] Use spacing tokens and give TabPanel the 8px item gap - #4088
Open
idaiv wants to merge 2 commits into
Open
[Form Kit] Use spacing tokens and give TabPanel the 8px item gap#4088idaiv wants to merge 2 commits into
idaiv wants to merge 2 commits into
Conversation
Two things, both toward the consistent 8px field rhythm asked for in #3371. Spacing now comes from tokens instead of literals: - `padding: '8px 16px'` (7x across classification-store-config) -> `${token.paddingXS}px ${token.padding}px` — same pixels. - Region's `GAP_SIZE = 12` -> `token.sizeSM`. Same 12px, but it feeds both the grid gap and the percentage width correction, so the two can no longer drift apart. Left at 12px deliberately: this is a region/column gap, a different axis from field rhythm. - `itemMarginBottom: 8` in the language comparison -> `token.marginXS`. - Field descriptions in the appearance color panel: `marginTop: 8`, `fontSize: '12px'` and `color: '#666'` -> marginXS / fontSizeSM / colorTextSecondary. - Two strays: a reports divider at 10px -> `marginXS` (8px, matching the tokens already used around it), and the thumbnail empty states at 50px -> `marginXXL` (48px, the nearest step). TabPanel now spaces its own children: FormKit sets antd's `itemMarginBottom` to 0 and expects the gap to come from `ItemSpacer`, but only `Panel` supplied one. Fields placed straight into a tab rendered flush — visible in the class editor, where the Text layout type's Configuration tab stacks three inputs with no gap at all. TabPanel now wraps its content in `ItemSpacer`, mirroring Panel. This is safe for existing callers because gap only applies between siblings: `object-tabpanel` passes one component per tab and every story wraps tabs in a Panel, so both are unchanged. The GDPR extractor has its own local Tabpanel and is untouched. Storybook: the TabPanel story documents the contract, and a new `FieldsWithoutPanel` story covers the shape that used to break, next to a single-field tab showing nothing doubles up. Refs #3371 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
FormKit-specific spacing must not alter the shared TabPanel behavior for non-FormKit consumers.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Tokenizes form-related spacing and adds 8px spacing between direct TabPanel children.
Changes:
- Replaces spacing literals with theme tokens.
- Adds
ItemSpacerto TabPanel content. - Adds Storybook documentation and examples.
File summaries
| File | Review |
|---|---|
assets/js/src/core/modules/video-thumbnails/video-thumbnails-container.styles.ts |
Tokenizes empty-state margin. |
assets/js/src/core/modules/reports/reports-view/reports-view.styles.tsx |
Tokenizes divider spacing. |
assets/js/src/core/modules/image-thumbnails/image-thumbnails-container.styles.ts |
Tokenizes empty-state margin. |
assets/js/src/core/modules/data-object/editor/toolbar/language-comparison-view/language-comparison-content.tsx |
Tokenizes form-item spacing. |
assets/js/src/core/modules/classification-store-config/components/store-editor/tabs/keys-tab.tsx |
Tokenizes toolbar padding. |
assets/js/src/core/modules/classification-store-config/components/store-editor/tabs/groups/key-group-relations-grid.tsx |
Tokenizes toolbar padding. |
assets/js/src/core/modules/classification-store-config/components/store-editor/tabs/groups-tab.tsx |
Tokenizes panel padding. |
assets/js/src/core/modules/classification-store-config/components/store-editor/tabs/collections/collection-group-relations-grid.tsx |
Tokenizes toolbar padding. |
assets/js/src/core/modules/classification-store-config/components/store-editor/tabs/collections-tab.tsx |
Tokenizes panel padding. |
assets/js/src/core/modules/appearance-branding/components/appearance-form/components/color-panel/color-panel.tsx |
Tokenizes helper text, but the chosen font and color tokens are not pixel-identical. |
assets/js/src/core/components/tabpanel/tabpanel.tsx |
Adds spacing at the shared API boundary, unintentionally affecting non-FormKit consumers. |
assets/js/src/core/components/region/region.styles.ts |
Tokenizes the region gap. |
assets/js/src/core/components/form/layouts/tabpanel/tabpanel.stories.tsx |
Documents and demonstrates direct-field spacing. |
Review details
Suppressed comments (2)
assets/js/src/core/modules/appearance-branding/components/appearance-form/components/color-panel/color-panel.tsx:66
- This is not pixel-identical under the repository's default theme. The theme seeds
fontSize: 12(dynamic-type-theme-studio-default-light.ts:71), and Ant Design 5.22 derivesfontSizeSMas 10px; it also derivescolorTextSecondaryat 65% black, whereas#666is 60% black on white. This makes the helper text smaller and darker despite the PR stating this row is unchanged. Use the existing 12px/60%-black semantic tokens to preserve the current rendering.
<div style={ { marginTop: token.marginXS, fontSize: token.fontSizeSM, color: token.colorTextSecondary } }>
assets/js/src/core/modules/appearance-branding/components/appearance-form/components/color-panel/color-panel.tsx:87
- As above,
fontSizeSMresolves to 10px from this theme's 12px seed andcolorTextSecondaryresolves to 65% black, so this second description also changes from 12px/#666. UsefontSizeand the theme's explicitly configured 60%-blackcolorTextTertiarytoken if this conversion is meant to remain visually unchanged.
<div style={ { marginTop: token.marginXS, fontSize: token.fontSizeSM, color: token.colorTextSecondary } }>
- Files reviewed: 13/14 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+85
to
+87
| <ItemSpacer> | ||
| {item.children} | ||
| </ItemSpacer> |
|
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
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.
Refs #3371
Scope, and what this does not do
#3371 asks for a consistent 8px vertical gap rule in the form kit. This PR does two things toward that — moves spacing onto tokens, and fixes the one place where the 8px gap was silently not applied at all.
It deliberately does not change the value that governs most forms.
studio-default-light.tssetsForm.itemMarginBottom: 12, and 227 files use plainForm.Itemagainst 69 usingFormKit— so the majority regime in the app is still 12px, not 8px. Changing it shifts every non-FormKit form by 4px and needs a visual pass, so it is left for a follow-up. The issue should stay open after this merges.Tokens instead of literals
padding: '8px 16px'×7 (classification-store-config)`${token.paddingXS}px ${token.padding}px`GAP_SIZE = 12(Region)token.sizeSMitemMarginBottom: 8(language comparison)token.marginXSmarginTop: 8/fontSize: '12px'/color: '#666'×2 (appearance color panel)marginXS/fontSizeSM/colorTextSecondarymargin-bottom: 10px(reports group divider)token.marginXSmargin-top: 50px×2 (thumbnail empty states)token.marginXXLEverything except the last two rows is pixel-identical by construction.
Regionstays at 12px on purpose — it is a region/column gap feeding acalc()width correction, a different axis from field rhythm; tokenising it means the gap and the correction can no longer drift apart.TabPanel now supplies the item gap
FormKitsetsitemMarginBottom: 0and expects the gap fromItemSpacer, but onlyPanelsupplied one. Fields placed straight into a tab therefore rendered flush — live example in the class editor: the Text layout type's Configuration tab stacksrenderingClass,renderingDataand the WYSIWYG with no gap (field-definition-text-form-fields.tsx).Tabpanelnow wraps its content inItemSpacer, mirroringPanel. Fixed at the component rather than per-caller because gap only applies between siblings, so every existing consumer is unaffected:object-tabpanel— one<ObjectComponent>per tab → no change<Panel>per tab → no changegdpr-data-extractor— its own localTabpanel(takesdata/executeSearch) → untouchedStorybook
The TabPanel story now states the contract (8px via
ItemSpacer, why it is needed givenitemMarginBottom: 0, and that Panel-wrapping does not double up), plus a newFieldsWithoutPanelstory rendering the shape that used to break, beside a single-field tab so the no-doubling case is visible too.Testing
Lint clean on all 13 files; 40 tests pass in
components/form,components/tabpanel,components/panel;npm run build-appsucceeds.Not verified visually, and worth a reviewer's eye:
ItemSpacerintroduces aFlex verticalwrapper inside every tab. Gaps are provably unchanged for single-child tabs, but a flex context can alter how height and overflow propagate, andobject-tabpanelfeedsObjectComponentthrough it in the data-object editor. Tests will not catch that. A look at a data object's edit view would settle it.