Feat/preview style: Modify the style to be consistent with toolbar save. #1709
Conversation
WalkthroughToolbar UI and behavior changes: ToolbarBaseButton rendering and styles adjusted; preview and save toolbars refactored to use TinyPopover with manual visibility and up/down icons; Logo toolbar entry removed from registry; ToolbarBase click handler now stops propagation; package.json peer/dependencies updated for preview/save. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧠 Learnings (4)📓 Common learnings📚 Learning: 2024-09-30T07:51:10.036ZApplied to files:
📚 Learning: 2025-01-14T06:49:00.797ZApplied to files:
📚 Learning: 2025-01-14T06:49:00.797ZApplied to files:
⏰ 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)
🔇 Additional comments (1)
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: 1
🧹 Nitpick comments (1)
packages/design-core/registry.js (1)
20-21: Logo toolbar removal matches described workaroundCommenting out the
Logoimport and itsengine.toolbars.logoentry cleanly removes the Logo item from the toolbar, which should eliminate the blank clickable block caused by the missing icon. As this is a temporary removal, consider adding a small TODO near one of these comments to track re‑enabling once the Logo component is fixed, but functionally this change is fine.Also applies to: 141-142
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/common/component/toolbar-built-in/ToolbarBaseButton.vue(3 hunks)packages/design-core/registry.js(2 hunks)packages/toolbars/preview/meta.js(1 hunks)packages/toolbars/preview/src/Main.vue(3 hunks)packages/toolbars/save/src/Main.vue(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: gene9831
Repo: opentiny/tiny-engine PR: 1041
File: packages/plugins/datasource/src/DataSourceList.vue:138-138
Timestamp: 2025-01-14T10:06:25.508Z
Learning: PR #1041 in opentiny/tiny-engine is specifically for reverting Prettier v3 formatting to v2, without any logical code changes or syntax improvements.
📚 Learning: 2024-10-10T02:47:46.239Z
Learnt from: yy-wow
Repo: opentiny/tiny-engine PR: 850
File: packages/toolbars/preview/src/Main.vue:16-16
Timestamp: 2024-10-10T02:47:46.239Z
Learning: In `packages/toolbars/preview/src/Main.vue`, within the `preview` function, the `getMergeMeta` method is used at lines 64 and 65 to retrieve `engine.config` configurations.
Applied to files:
packages/toolbars/preview/meta.jspackages/toolbars/preview/src/Main.vue
📚 Learning: 2024-10-10T02:48:10.881Z
Learnt from: yy-wow
Repo: opentiny/tiny-engine PR: 850
File: packages/toolbars/preview/src/Main.vue:0-0
Timestamp: 2024-10-10T02:48:10.881Z
Learning: 在 `packages/toolbars/preview/src/Main.vue` 文件中，使用 `useNotify` 而不是 `console` 来记录错误日志。
Applied to files:
packages/toolbars/preview/src/Main.vuepackages/common/component/toolbar-built-in/ToolbarBaseButton.vue
📚 Learning: 2025-01-14T06:49:00.797Z
Learnt from: gene9831
Repo: opentiny/tiny-engine PR: 1011
File: packages/configurator/src/router-select-configurator/RouterSelectConfigurator.vue:63-73
Timestamp: 2025-01-14T06:49:00.797Z
Learning: In the tiny-engine project, the SvgIcon component is globally registered and available throughout Vue components without requiring explicit imports.
Applied to files:
packages/toolbars/preview/src/Main.vue
⏰ 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: push-check
🔇 Additional comments (5)
packages/toolbars/save/src/Main.vue (1)
10-10: Confirm arrow visibility change on save popoverThe
tiny-popoverhere no longer passesvisible-arrow, so its arrow behavior has changed compared to the previous implementation. Please confirm this is intentional and aligned with the updated toolbar preview design; otherwise, we may need to restore explicit arrow control or add the appropriate class to match the new.no-arrowstyling.packages/common/component/toolbar-built-in/ToolbarBaseButton.vue (1)
6-6: Title visibility controlled viaoptions.showTitlelooks goodConditionally rendering the title span when
options?.showTitle !== falsecleanly supports hiding titles (e.g., for preview) while keeping existing buttons unchanged.packages/toolbars/preview/meta.js (1)
9-10: Preview meta now correctly drives button + icon-only renderingSwitching
renderTypeto'button'and addingshowTitle: falsealigns preview with the new ToolbarBaseButton behavior, giving an icon-only button while reusing the common base styling.packages/toolbars/preview/src/Main.vue (2)
2-9: Preview toolbar popover UX: verify combined hover/click behaviorThe new structure—button-style
toolbar-basewithtrigger="hover"and a nested hover-triggeredtiny-popoveroffering “页面预览/应用预览”—matches the desired preview dropdown pattern, with a default click still doingpreview('page').Please sanity‑check the UX in the browser to ensure:
- Hover on the preview control reliably opens the popover and keeps it open long enough to click “页面预览/应用预览”.
- The
trigger="hover"ontoolbar-baseand ontiny-popoverdon’t conflict (e.g., no flicker or unexpected close when moving from the button to the popover content).If everything feels smooth, the structure looks good.
Also applies to: 11-19
29-35: Popover wiring and props integration look consistent
- Importing
constantsand exposingOPEN_DELAYfor:open-delay="OPEN_DELAY.Default"keeps preview aligned with the save toolbar’s timing.- Registering
TinyPopover: Popover as Componentand adding theiconExpandprop (default'down-arrow') are correctly reflected in the template.- The
previewfunction continues to useuseNotifyfor error reporting instead ofconsole, which matches prior project guidance. Based on learnings, this is the preferred pattern here.No issues from a logic or wiring perspective.
Also applies to: 38-39, 42-45, 96-99
| background-color: var(--te-component-toolbar-base-button-bg-color) !important; | ||
| border: none !important; | ||
| min-width: 70px; | ||
| min-width: 60px; |
There was a problem hiding this comment.
❓ Verification inconclusive
🏁 Script executed:
find . -name "ToolbarBaseButton.vue" -type fRepository: opentiny/tiny-engine
Length of output: 184
🌐 Web query:
Vue.js scoped CSS slotted content does not apply scope attribute
💡 Result:
Cause — scoped styles are only applied to elements owned by the component (Vue adds a data-v-xxx attribute to those elements). Content passed into a is owned by the parent, so Vue does not add the component’s scope attribute to slotted nodes and scoped rules don’t match them by default. [1][2]
Fixes
- Use the :slotted() pseudo-class inside the child component’s <style scoped> to target slotted nodes: <style scoped> :slotted(.my-item) { color: red; } </style>
- Or move the rule to the parent (where the slotted content lives) or use a non-scoped/global style.
- For styling elements inside child components or deep descendants, use :deep(...) (not for slotted content owned by the parent). [1][2]
References
[1] Vue SFC CSS features (scoped, :slotted, :deep).
[2] vue-loader — Scoped CSS behavior and caveats.
Scoped .tiny-popover.no-arrow rule won't apply to slotted popovers
Since popovers are provided via slots, they're owned by parent components and won't carry this component's scope attribute. The scoped .tiny-popover.no-arrow rule won't target them.
Consider using the :slotted() pseudo-class if styling direct slot children, moving this rule to the parent component where the popover is rendered, or using a global/non-scoped style file instead. Avoid :deep() for slotted content, as it's for styling child components and descendants, not slot-provided elements.
🤖 Prompt for AI Agents
In packages/common/component/toolbar-built-in/ToolbarBaseButton.vue around line
39, the scoped CSS rule targeting .tiny-popover.no-arrow won't apply to popovers
rendered via slots because slotted content is owned by the parent and doesn't
get this component's scope attribute; update the styling by either (a) moving
the .tiny-popover.no-arrow rule into the parent component that renders the
popover, (b) place the rule in a global/non-scoped stylesheet so it applies to
slotted elements, or (c) if you only need to style direct slot children, use the
:slotted(.tiny-popover.no-arrow) selector in this component's <style scoped>
block; avoid using :deep() for slot-provided elements.
1 participant
English | 简体中文
PR
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
Background and solution
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
New Features
Style
Bug Fixes / Accessibility
Chores
✏️ Tip: You can customize this high-level summary in your review settings.