Repository navigation
fix(Tabs): apply styles.popup.root to the more dropdown - #59221
Conversation
|
|
|
|
👁 Visual Regression Report for PR #59221 Passed ✅
🎊 Congrats! No visual-regression diff found. 
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 SummarySummary by CodeRabbit
WalkthroughTabs 将语义化配置中的 ChangesTabs 弹出层语义样式
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to Tabs more-menu dropdowns now receive configured popup root styles, with coverage confirming the rendered class and color styling. No current merge-blocking risk remains. Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
More templates
commit: |
Bundle ReportChanges will decrease total bundle size by 5.77MB (-64.09%) ⬇️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: antd.min-array-pushAssets Changed:
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #59221 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 834 834
Lines 15994 15991 -3
Branches 4204 4202 -2
=========================================
- Hits 15994 15991 -3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
nrps9909
left a comment
nrps9909
left a comment
There was a problem hiding this comment.
Reviewed exact head f91167d0c96b96af3926a20fbd3ba1ad12b61d03 against base 58b0ee1554835b3ea2d0541f4986306f3d70d0ce. The projection of mergedStyles.popup.root matches rc-tabs' flat popup style contract; I found no blocker in this scoped change.
Validation:
- The new overflow-popup regression fails against the base implementation and passes on the PR head. The four semantic tests also pass under Vitest.
- Two independent DOM probes pass: provider-only popup fallback, local popup precedence, merged popup class names, preservation of root/header styles, callback evaluation, and clearing obsolete inline styles after rerenders.
- The complete Tabs + ConfigProvider run reports 663 passed, 4 skipped, and one failed test; 454 snapshots pass and one differs. The remaining
tabs/demo/nest.tsxextend-context snapshot gains Selectaria-posinset/aria-setsizeattributes. The exact base produces the identical 16-line diff with the same installed dependencies. I did not update that snapshot or count this as a clean full-suite pass. - ESLint, Biome and diff checks pass for the changed files. All 42 applicable remote checks pass; two are skipped.
Separate existing behavior observed during probing: when both ConfigProvider and Tabs supply styles.popup.root, the shared mergeStyles helper replaces the nested root object, so unrelated provider properties are lost. That helper is unchanged by this PR. Provider-only fallback and the local styles work with this fix; property-level merging of nested style slots would be a separate follow-up.
Review and independent validation performed with OpenAI Codex assistance.
| it('support popup classNames and styles', async () => { | ||
| // The more dropdown only mounts when the tabs overflow, so fake the layout | ||
| // Longest first: `ant-tabs-nav` is a prefix of the other two | ||
| const sizes: Record<string, number> = { |
There was a problem hiding this comment.
Could we open the dropdown directly by configuring more, then verify the popup's class and style? This test mainly checks whether the styles are applied, so it should not need to simulate element dimensions, tab overflow, or resize and hover events. Could we simplify the test setup?
There was a problem hiding this comment.
Updated. The test now passes more={{ visible: true }} and opens the popup directly.
In rc-tabs OperationNode, more is spread onto the Dropdown after its own visible: tabs.length ? open : false, so more.visible wins and the popup mounts without any overflowing tabs. That removed the offsetWidth/offsetHeight/offsetLeft prototype patching, the resize trigger, the mouseEnter event and the fake timers. The test is synchronous now and asserts the same class and style on .ant-tabs-dropdown.
Coverage is unchanged: with components/tabs/index.tsx reverted to the base version, only the style assertion fails (color: var(--ant-color-text) instead of rgb(255, 0, 0)) while the class assertion still passes. Green under both jest and vitest.
d0977d1 to
5d471ce
Compare
| const customClassnames = { | ||
| root: 'test-class', | ||
| item: 'test-item', | ||
| remove: 'test-remove', | ||
| indicator: 'test-indicator', | ||
| header: 'test-header', | ||
| body: 'test-body', | ||
| content: 'test-content', | ||
| }; | ||
| const customStyles = { | ||
| root: { color: 'rgb(255, 0, 0)' }, | ||
| item: { color: 'rgb(0, 0, 255)' }, | ||
| remove: { color: 'rgb(255, 0, 255)' }, | ||
| indicator: { color: 'rgb(255, 255, 0)' }, | ||
| header: { color: 'rgb(0, 255, 0)' }, | ||
| body: { color: 'rgb(0, 128, 128)' }, | ||
| content: { color: 'rgb(128, 0, 128)' }, | ||
| }; |
There was a problem hiding this comment.
Can the new case be added directly to this case?
There was a problem hiding this comment.
Done, folded into support classnames and styles. popup: { root: ... } now sits in the existing customClassnames and customStyles objects, the render gets more={{ visible: true }} so the dropdown mounts, and the two assertions moved in beside the others. The separate case is gone.
One thing worth a look: the popup is portaled, so it is read off document.body rather than container like the rest.
Counterfactual still holds. With components/tabs/index.tsx reverted, the case fails on color: var(--ant-color-text) instead of rgb(0, 255, 255). components/tabs goes from 88 to 87 tests, no snapshot change.
|
LGTM。这里参照 LGTM. This handles |
🤔 This is a ...
🔗 Related Issues
No linked issue. The problem is visible in the repository itself:
components/tabs/demo/_semantic.tsxsetsstyles={{ popup: { root: { background: '#fff' } } }}, and that value never reaches the DOM.💡 Background and Solution
Tabsdeclares the popup semantic node as a nested object on both sides (components/tabs/index.tsx):@rc-component/tabskeepspopupflat (Partial<Record<SemanticName, string>>/Partial<Record<SemanticName, CSSProperties>>), so the nested value has to be unwrapped on the way down.classNamesis unwrapped:stylesis not.mergedStyles.popuptherefore arrives at rc-tabs as{ root: CSSProperties }, is forwarded aspopupStyle->overlayStyleon the more dropdown, and React drops it becauserootis not a CSS property. The result is thatclassNames.popup.rootworks andstyles.popup.rootsilently does nothing on the same element.stylesis now unwrapped the same wayclassNamesalready is. This is how every sibling component with the same nested popup semantic already behaves, for examplecomponents/select/index.tsx(popupStyle={{ ...mergedStyles.popup.root, ... }}),components/cascader/index.tsxandcomponents/auto-complete/AutoComplete.tsx.Nothing else changes: the layout, the public API, and the
classNamespath are untouched, and no snapshot moves because the more dropdown is not opened in any snapshot test.components/tabs/__tests__/semantic.test.tsxgains a case that overflows the tab bar, opens the more dropdown and asserts both halves on the popup element. Reverting onlycomponents/tabs/index.tsxturns it red on the style assertion while the class assertion above it still passes:Verification:
npx jest --config .jest.js --no-cache components/tabs components/config-provider-> 27 suites passed, 662 tests passed, 455 snapshots passed, 0 snapshots written.npx vitest run components/tabs/__tests__/semantic.test.tsx-> 4 passed.npm run tsc,eslint,biome lint,biome format-> clean.📝 Change Log
styles.popup.rooton the more dropdown.styles.popup.root的问题。