fix: resolve the issue of button size not expanding with text #3717
Conversation
WalkthroughChanged guide button styling from a fixed width to a flexible minimum width and added horizontal padding by updating theme guide variables and styles. No JS logic, exports, or control flow were modified. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
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 (2)
packages/theme/src/guide/index.less (2)
140-152: Good fix: min-width unblocks auto-expansion with longer labels.Switching from width to min-width aligns with the PR goal and keeps the default minimum. Looks safe with the flex container.
Please sanity-check:
- Very long labels in narrow steps (width: var(--tiny-guide-shepherd-element-size)) don’t overflow the container.
- RTL locales and CJK text still render without clipping given the fixed height.
Optional hardening (prevents overflow without changing visuals in normal cases):
.shepherd-button { color: var(--tv-Guide-button-text-color); background-color: var(--tv-Guide-button-bg-color); cursor: pointer; border-radius: var(--tv-Guide-button-border-radius); border: 1px solid var(--tv-Guide-button-border-color); min-width: var(--tv-Guide-button-width); + max-width: 100%; + white-space: nowrap; + text-overflow: ellipsis; + overflow: hidden; height: var(--tv-Guide-button-height); margin-left: 8px; text-align: center; transition: all 0.5s ease; font-size: var(--tv-Guide-button-font-size); }
128-131: Selector bug: rule never matches; likely intended descendant selector.&.shepherd-button:last-child targets an element that is both shepherd-footer and shepherd-button, which won’t occur. Either drop the rule (since spacing uses margin-left) or fix to target the last button.
Minimal fix (keeps the intent consistent with margin-left spacing):
- &.shepherd-button:last-child { - margin-right: 0; - } + .shepherd-button:first-child { + margin-left: 0; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/theme/src/guide/index.less(1 hunks)
⏰ 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)
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/theme/src/guide/vars.less (1)
67-67: Unify typo'd CSS variable--tv-Guide-progresss-text-color→--tv-Guide-progress-text-colorDuplicate/typo found in packages/theme/src/guide/vars.less (line 41 vs line 67); packages/theme/src/guide/index.less (line 136) references the typo — unify tokens and update usages.
Preferred (keep canonical name): update usage and remove duplicate
packages/theme/src/guide/index.less - color: var(--tv-Guide-progresss-text-color); + color: var(--tv-Guide-progress-text-color);packages/theme/src/guide/vars.less - --tv-Guide-progresss-text-color: var(--tv-color-text-weaken, #808080);Alternative (if the weakened color is intended): set the canonical token to the weakened value and remove the typo line
packages/theme/src/guide/vars.less - --tv-Guide-progress-text-color: var(--tv-color-text, #191919); + --tv-Guide-progress-text-color: var(--tv-color-text-weaken, #808080); - --tv-Guide-progresss-text-color: var(--tv-color-text-weaken, #808080);
🧹 Nitpick comments (2)
packages/theme/src/guide/vars.less (2)
62-63: Fix mislabeled comment for the new token.The comment repeats “按钮高度” but the variable defines horizontal padding. Correct the label to avoid confusion.
- // 引导框按钮高度 + // 引导框按钮水平内边距 --tv-Guide-button-padding-horizontal: 16px;
62-63: Consider tying padding to the spacing scale (if available).If a global spacing token exists (e.g.,
--tv-spacing-md), prefer it with a 16px fallback to keep Guide aligned with design tokens.Proposed (only if such token exists):
- --tv-Guide-button-padding-horizontal: 16px; + --tv-Guide-button-padding-horizontal: var(--tv-spacing-md, 16px);Please confirm the correct spacing token name in this repo’s system.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/theme/src/guide/index.less(1 hunks)packages/theme/src/guide/vars.less(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/theme/src/guide/index.less
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-11-04T09:35:13.159Z
Learnt from: zzcr
PR: opentiny/tiny-vue#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/guide/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 (1)
packages/theme/src/guide/vars.less (1)
59-61: Call out semantic change: width var now used as min-width downstream.If
--tv-Guide-button-widthis switching from width to min-width in index.less, note this in tokens/docs to prevent surprises for theme overrides relying on exact width.Do we want a follow-up doc snippet stating “Guide button width token is applied as min-width at runtime”?
PR
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
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