fix(space): [space]default spacing for overlay button components #3816
Conversation
WalkthroughRemoved inline per-button margin attributes from demo Space examples and added a theme-level Space stylesheet and variables; the Vue Space entry now imports the new theme stylesheet, and theme index aggregates the Space styles. Changes
Sequence Diagram(s)(omitted — changes are styling and imports; no runtime control-flow change to diagram) Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/theme/src/space/index.less (1)
20-20: TODO indicates incomplete solution - consider tracking.The TODO comment mentions that button component spacing variables need to be unified in a future major version. This suggests the current implementation is a temporary workaround.
Consider creating a tracking issue for the future alignment work mentioned in the TODO. Would you like me to help draft an issue description for unifying Space and Button spacing in a future major version?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
examples/sites/demos/mobile-first/app/space/space-size.vue(1 hunks)examples/sites/demos/pc/app/space/space-size.vue(1 hunks)packages/theme/src/space/index.less(1 hunks)packages/theme/src/space/vars.less(1 hunks)packages/theme/src/vars.less(1 hunks)packages/vue/src/space/index.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-11-04T09:35:13.159Z
Learnt from: zzcr
Repo: opentiny/tiny-vue PR: 2481
File: packages/theme/src/time-range/vars.less:27-28
Timestamp: 2024-11-04T09:35:13.159Z
Learning: 在 `packages/theme/src/time-range/vars.less` 文件中，应使用 `var(--tv-TimeRange-header-height)` 作为 `--tv-TimeRange-header-line-height` 的值，以保持一致性。
Applied to files:
packages/theme/src/space/index.lesspackages/theme/src/space/vars.less
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: PR E2E Test (pnpm test:e2e3)
🔇 Additional comments (6)
examples/sites/demos/mobile-first/app/space/space-size.vue (1)
18-18: LGTM - consistent with PC demo.The removal of inline margin styling matches the changes in the PC demo and correctly delegates spacing control to the theme layer.
packages/theme/src/vars.less (1)
55-55: LGTM - correct alphabetical placement.The import of
./space/vars.lessis properly placed in alphabetical order between split and steps, consistent with the existing import pattern.packages/vue/src/space/index.ts (1)
14-14: LGTM - necessary theme integration.The import of the Space theme stylesheet ensures the new spacing styles are loaded when the component is used. The placement after the component import is appropriate.
packages/theme/src/space/index.less (1)
21-23: Rewrite the review comment: The&&specificity boost and zero margin are intentional and documented.The code uses
&&to override the button component's default& + &margin rule, setting it to0pxso the Space component's flexboxgapproperty can manage spacing instead. This is a temporary workaround documented in the TODO comment, which states it will be removed once the button component removes its default 8px margin in a future major version.The implementation is not a specificity conflict but a known pattern: the Space component uses modern CSS flexbox gap for spacing, while this rule suppresses button margins that would otherwise apply. The approach correctly isolates the override to
.tiny-buttonelements within Space contexts, with a clear plan for cleanup.No changes needed.
packages/theme/src/space/vars.less (1)
13-15: The review comment is based on a misunderstanding of the code's purpose.The
--tv-Space-margin: 0pxsetting applies only to buttons within a Space container (per the CSS rule atpackages/theme/src/space/index.less:21-23). It doesn't override Button component defaults; rather, it gives the Space component control over its children's spacing.The TODO comment at
packages/theme/src/space/index.less:20explicitly documents this as temporary: spacing should be unified with Button in a future major version. The architectural intent is clear: Space should have its own spacing control separate from Button's adjacent-button margins (--tv-Button-margin-left-btn-to-btn-md).Referencing Button spacing variables in Space margin would violate separation of concerns and is not necessary. The current design is sound and addresses the alignment concern through the documented TODO.
Likely an incorrect or invalid review comment.
examples/sites/demos/pc/app/space/space-size.vue (1)
18-18: Code structure is correct; recommend manual testing to confirm visual spacing.The Space component correctly applies the
class="tiny-space"from the demo, and the theme CSS rule&& .tiny-button + .tiny-button { margin-left: var(--tv-Space-margin) }will properly target adjacent buttons. The--tv-Space-margin: 0pxvariable cancels default button spacing, while the Space component's inlinegapstyle (from the:sizeprop) provides the actual spacing via flexbox.Verify visually that buttons render with correct spacing when the demo's row/column sliders adjust the size values.
3 participants
PR
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
--覆盖默认的button组件样式
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Does this PR introduce a breaking change?
Other information
Summary by CodeRabbit