chore: 新增和优化一些组件功能，详见提交信息 #6830
Conversation
zouawen
requested review from
a team,
anncwb,
jinmao88,
mynetfan and
vince292007
as code owners
|
WalkthroughThis PR includes styling refinements across UI components, authentication panel layout restructuring with dynamic data-side attributes, lock screen modal autofocus integration via getFieldComponentRef, handler refactoring in the modal component, and updates to mock data parent-child references and playground examples. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Modal as Lock Screen Modal
participant Form as useVbenForm
participant Input as Password Input
User->>Modal: Opens lock screen
Modal->>Modal: Modal opens (onOpened triggered)
Modal->>Form: getFieldComponentRef('lockScreenPassword')
Form-->>Modal: Returns field component ref
Modal->>Input: input.focus()
Input-->>User: Focus acquired on password field
Note over Modal,Input: Auto-focus enables passwordfield on modal open
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes The PR combines straightforward style tweaks (Tailwind utilities, animation adjustments) with moderate complexity refactors in authentication panel layout restructuring and lock screen modal integration. While changes span 13 files, they follow consistent patterns within cohorts, but require verification of animation behavior, modal lifecycle timing, and authentication state management. Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/@core/ui-kit/popup-ui/src/modal/modal.vue (1)
183-187: Critical: drawer.vue has the same typo and needs fixing.The rename in modal.vue is correct. However, verification found that
packages/@core/ui-kit/popup-ui/src/drawer/drawer.vuestill has the old typo:
- Line 138: Function is still named
handerOpenAutoFocus(should behandleOpenAutoFocus)- Line 204: Template still references the old typo:
@open-auto-focus="handerOpenAutoFocus"Apply the same rename fix to drawer.vue to keep both files consistent.
🧹 Nitpick comments (8)
packages/@core/ui-kit/shadcn-ui/src/ui/checkbox/Checkbox.vue (1)
34-34: Avoid hover highlight on disabled checkboxes
hover:border-primarywill still apply when disabled. Prevent misleading affordance.Apply one of:
- '... border-border hover:border-primary peer h-4 w-4 shrink-0 rounded-sm border transition focus-visible:outline-none focus-visible:ring-1 disabled:cursor-not-allowed disabled:opacity-50' + '... border-border hover:border-primary peer h-4 w-4 shrink-0 rounded-sm border transition focus-visible:outline-none focus-visible:ring-1 disabled:pointer-events-none disabled:cursor-not-allowed disabled:opacity-50'Or neutralize hover explicitly:
- '... hover:border-primary ...' + '... hover:border-primary disabled:hover:border-border ...'packages/@core/ui-kit/shadcn-ui/src/ui/input/Input.vue (1)
27-27: Verify placeholder contrast at 50% opacity
placeholder:text-muted-foreground/50may drop below WCAG contrast in certain themes. Consider slightly stronger contrast (e.g.,/60or/70) orplaceholder:opacity-70, and validate in light/dark and high-contrast modes.packages/@core/base/design/src/css/ui.css (1)
2-2: Add reduced-motion fallback for larger entrance animationsWith longer duration and 50px travel, respect users’ motion preferences.
Add a reduced-motion override:
+@media (prefers-reduced-motion: reduce) { + .side-content { + animation: none !important; + } + .breadcrumb-transition-enter-active { + transition: none !important; + } +}Optional perf nit:
-.side-content { +.side-content { animation-duration: 0.3s; animation-timing-function: cubic-bezier(0.16, 1, 0.3, 1); + will-change: transform, opacity; }Also applies to: 38-47, 49-59, 61-71, 73-83
packages/@core/ui-kit/popup-ui/src/modal/modal.vue (1)
212-216: Guard onOpened to avoid races if closed before rAF fires.If the modal is closed quickly, onOpened could run after close. Add an open-state check inside rAF.
-const handleOpened = () => { - requestAnimationFrame(() => { - props.modalApi?.onOpened(); - }); -}; +const handleOpened = () => { + requestAnimationFrame(() => { + if (state?.value?.isOpen) { + props.modalApi?.onOpened(); + } + }); +};packages/effects/layouts/src/widgets/lock-screen/lock-screen.vue (1)
78-84: Harden focus routine (fallback + preventScroll) and consider reuse.Use a fallback selector and prevent scroll; same pattern exists in lock-screen-modal.vue — consider extracting a tiny helper.
- if (showUnlockForm.value) { - requestAnimationFrame(() => { - getFieldComponentRef('password') - ?.$el?.querySelector('[name="password"]') - ?.focus(); - }); - } + if (showUnlockForm.value) { + requestAnimationFrame(() => { + const root = getFieldComponentRef('password')?.$el as HTMLElement | undefined; + const input = + root?.querySelector<HTMLInputElement>('[name="password"]') ?? + root?.querySelector<HTMLInputElement>('input'); + input?.focus({ preventScroll: true }); + }); + }packages/effects/layouts/src/widgets/lock-screen/lock-screen-modal.vue (2)
30-53: Form helpers LGTM; unify validation message key.Schema and helpers look good. For consistency with lock-screen.vue, consider using authentication.passwordTip as the min rule message.
- rules: z - .string() - .min(1, { message: $t('ui.widgets.lockScreen.placeholder') }), + rules: z + .string() + .min(1, { message: $t('authentication.passwordTip') }),
64-70: Improve autofocus robustness (fallback + preventScroll) and dedupe.Same as lock-screen.vue: add input fallback and preventScroll; optionally extract a shared helper.
- onOpened() { - requestAnimationFrame(() => { - getFieldComponentRef('lockScreenPassword') - ?.$el?.querySelector('[name="lockScreenPassword"]') - ?.focus(); - }); - }, + onOpened() { + requestAnimationFrame(() => { + const root = getFieldComponentRef('lockScreenPassword')?.$el as HTMLElement | undefined; + const input = + root?.querySelector<HTMLInputElement>('[name="lockScreenPassword"]') ?? + root?.querySelector<HTMLInputElement>('input'); + input?.focus({ preventScroll: true }); + }); + },packages/effects/layouts/src/authentication/authentication.vue (1)
90-96: Transition wrapper is fine; consider reduced-motion.Optional: add motion-reduce classes to respect user preferences on transitions.
- class="flex-col-center mr-20 h-full" + class="flex-col-center mr-20 h-full motion-reduce:transition-none motion-reduce:animate-none"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
apps/backend-mock/utils/mock-data.ts(3 hunks)packages/@core/base/design/src/css/ui.css(5 hunks)packages/@core/ui-kit/layout-ui/src/vben-layout.vue(2 hunks)packages/@core/ui-kit/popup-ui/src/modal/modal.vue(3 hunks)packages/@core/ui-kit/shadcn-ui/src/ui/checkbox/Checkbox.vue(1 hunks)packages/@core/ui-kit/shadcn-ui/src/ui/input/Input.vue(1 hunks)packages/@core/ui-kit/shadcn-ui/src/ui/tabs/TabsList.vue(1 hunks)packages/effects/layouts/src/authentication/authentication.vue(4 hunks)packages/effects/layouts/src/authentication/form.vue(2 hunks)packages/effects/layouts/src/widgets/lock-screen/lock-screen-modal.vue(2 hunks)packages/effects/layouts/src/widgets/lock-screen/lock-screen.vue(2 hunks)playground/src/locales/langs/zh-CN/system.json(2 hunks)playground/src/views/examples/layout/col-page.vue(2 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). (3)
- GitHub Check: Check (windows-latest)
- GitHub Check: post-update (ubuntu-latest)
- GitHub Check: post-update (windows-latest)
🔇 Additional comments (12)
apps/backend-mock/utils/mock-data.ts (1)
277-303: LGTM! Correct parent-child reference fix.The
pidvalues for the three SystemDept child items (Create, Edit, Delete) have been correctly updated from 201 to 202, aligning them with their actual parent SystemDept (id: 202) rather than SystemMenu (id: 201). This fixes the data integrity issue in the menu hierarchy.playground/src/views/examples/layout/col-page.vue (2)
25-26: LGTM! Appropriate demo defaults.Setting
splitHandleandsplitLinetotrueby default is ideal for a playground example, as it immediately showcases these features to users exploring the component.
45-49: LGTM! Button styling improvement.The restructured Button component with centering classes improves the visual presentation of the expand control when the left panel is collapsed.
playground/src/locales/langs/zh-CN/system.json (2)
54-66: Nested title fields remain unchanged and consistent.The nested
"title"fields within thedept,menu, androlesections (lines 12, 46, 55 respectively) are preserved, maintaining consistency across the locale structure.
1-2: Root-leveltitlefield is correctly used by frontend code.Verification confirms the root-level
"title": "系统管理"field is actively consumed inplayground/src/router/routes/modules/system.ts(line 10) via the i18n call$t('system.title'). The change is functionally necessary and properly structured. The JSON syntax is correct with no semantic issues.packages/@core/ui-kit/shadcn-ui/src/ui/tabs/TabsList.vue (1)
24-24: LGTM — radius alignmentThe switch to
rounded-mdmatches other controls (e.g., inputs). No behavior change.packages/effects/layouts/src/authentication/form.vue (2)
6-8: New dataSide prop LGTM.Typed union is clear and safe.
23-24: Code changes verified—no issues found.CSS styling in
packages/@core/base/design/src/css/ui.cssproperly targets.side-contentwith[data-side]attribute selectors for all expected values (top,bottom,left,right). The Vue binding in form.vue is correct, and animations will trigger appropriately based on thedataSideprop value.packages/effects/layouts/src/authentication/authentication.vue (2)
53-53: Left panel data-side LGTM.Consistent with new prop.
120-120: Center panel data-side="bottom" LGTM.Matches the new layout semantics.
packages/@core/ui-kit/popup-ui/src/modal/modal.vue (1)
262-263: Event wiring verified as correct.All concerns confirmed:
- DialogContent emits "opened" (packages/@core/ui-kit/shadcn-ui/src/ui/dialog/DialogContent.vue:73)
- modal.vue template binding @opened="handleOpened" is consistent with the handler
- handleOpened handler is properly defined and invokes the callback chain
packages/effects/layouts/src/widgets/lock-screen/lock-screen.vue (1)
40-40: No issues found; getFieldComponentRef is properly exported and typed.Verification confirms that
getFieldComponentRefis:
- Defined as a public method in the
FormApiclass with correct signature- Included in the
ExtendedFormApitype via inheritance (ExtendedFormApi = FormApi & {...})- Properly returned from
useVbenFormand available for destructuring- Already in active use across multiple components with no TS drift
The review comment's concerns have been addressed by the current implementation.
| useLayoutHeaderStyle, | ||
| } from '@vben-core/composables'; | ||
| import { Menu } from '@vben-core/icons'; | ||
| import { IconifyIcon } from '@vben-core/icons'; |
There was a problem hiding this comment.
Icon state is bound to availability, not collapse; mobile handler only opens
- The icon condition uses
showSidebar(availability) instead of collapsed state, so it can display “fold” even when the sidebar is already collapsed, especially on mobile. - On mobile,
handleHeaderToggleonly opens (sidebarCollapse = false), so clicking again won’t close, while the “fold” icon suggests it would.
Fix by binding the icon to sidebarCollapse and truly toggling on mobile:
@@
- <VbenIconButton
+ <VbenIconButton
v-if="showHeaderToggleButton"
class="my-0 mr-1 rounded-md"
@click="handleHeaderToggle"
>
- <IconifyIcon v-if="showSidebar" icon="ep:fold" />
- <IconifyIcon v-else icon="ep:expand" />
+ <IconifyIcon :icon="sidebarCollapse ? 'ep:expand' : 'ep:fold'" />
</VbenIconButton>And update the handler to actually toggle on mobile:
@@
function handleHeaderToggle() {
- if (props.isMobile) {
- sidebarCollapse.value = false;
- } else {
+ if (props.isMobile) {
+ sidebarCollapse.value = !sidebarCollapse.value;
+ } else {
emit('toggleSidebar');
- }
+ }
}Also applies to: 562-564
🤖 Prompt for AI Agents
In packages/@core/ui-kit/layout-ui/src/vben-layout.vue around line 13 and also
at lines 562-564, the icon binding and mobile header toggle are incorrect:
change the icon condition to use the collapsed state (sidebarCollapse) instead
of showSidebar so the icon reflects collapse/expand, and modify
handleHeaderToggle to truly toggle sidebarCollapse on mobile (invert the
boolean) instead of only setting sidebarCollapse = false; apply the same fix to
the other occurrence at lines 562-564 so both icon rendering and mobile toggle
behavior are consistent.
| class="min-h-full w-2/5 flex-1" | ||
| data-side="right" |
There was a problem hiding this comment.
🧩 Analysis chain
Right panel width + data-side LGTM; verify breakpoints.
w-2/5 changes layout density; quickly validate on common breakpoints (sm/md/lg).
🏁 Script executed:
#!/bin/bash
# Grep for any conflicting width utilities around auth panels
rg -n -C2 -S $'AuthenticationFormView|w-2/5|w-1/2|w-1/3' --type=vueLength of output: 27
I need to correct the shell script approach since the --type=vue flag isn't recognized. Let me search for the file and examine its content:
🏁 Script executed:
#!/bin/bash
# Find the authentication.vue file
fd -e vue authentication
# If found, examine the file content around lines 136-137
if fd -e vue authentication | grep -q .; then
cat "packages/effects/layouts/src/authentication/authentication.vue" | head -150 | tail -30
fiLength of output: 798
🏁 Script executed:
#!/bin/bash
# Broader search for width utilities in Vue files (Tailwind classes)
rg -n 'w-2/5|w-1/2|w-1/3|flex-1|sm:w-|md:w-|lg:w-' packages/effects/layouts/src/authentication/ || echo "Search completed with no matches or directory not found"Length of output: 1222
Missing responsive breakpoint utilities on side panels.
The right panel at line 136 uses w-2/5 flex-1 without responsive variants. This fixed width applies across all breakpoints, causing layout density and usability issues on mobile/tablet screens (sm/md breakpoints).
Compare against the center form at line 119, which properly implements responsive widths: w-full md:w-2/3 lg:w-1/2 xl:w-[36%]. Similarly, the left panel at line 52 has the same issue.
Recommend applying responsive variants to both panels, e.g., w-full md:w-2/5 or a similar pattern aligned with the design system, so they adapt to smaller viewports.
🤖 Prompt for AI Agents
In packages/effects/layouts/src/authentication/authentication.vue around lines
136-137, the right panel uses fixed classes "w-2/5 flex-1" without responsive
variants causing poor mobile/tablet layouts; update the width classes to include
responsive breakpoints (for example use w-full md:w-2/5 or a design-aligned
pattern) so the panel collapses to full width on small screens while retaining
2/5 on medium+ screens; apply the same responsive adjustment to the left panel
(around line 52) to keep layout consistent.
|
Cool, can someone merge review and merge this? |
2 participants
Description
Type of change
Please delete options that are not relevant.
pnpm-lock.yamlunless you introduce a new test example.Checklist
pnpm run docs:devcommand.pnpm test.feat:,fix:,perf:,docs:, orchore:.Summary by CodeRabbit
New Features
Bug Fixes
Style
Chores