feat(dept): [dept] Optimize style #2487
Conversation
WalkthroughThe changes introduce a new Changes
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Outside diff range and nitpick comments (6)
packages/theme/src/espace/index.less (1)
25-25: Consider documenting spacing guidelines.Since this is part of a style optimization effort, it would be valuable to document the spacing system in a design system guide.
Consider adding a comment above the class definitions explaining the spacing rationale:
.item-talk, .item-call, .item-email { + // Use 24px horizontal spacing for better visual separation of communication items margin: 0 24px 0 0;examples/sites/demos/pc/app/hrapprover/custom-service-composition-api.vue (1)
9-9: Consider extracting the hardcoded title text for internationalization.While adding a title improves UI clarity, the Chinese text "选择权签人" is hardcoded. Since this appears to be a demo file, consider:
- Using an i18n key instead of hardcoded text
- Adding a comment explaining the title's meaning in English for non-Chinese speakers
Example modification:
- title="选择权签人" + :title="t('hrapprover.selectApprovalPerson')" <!-- Select Approval Person -->examples/sites/demos/pc/app/hrapprover/custom-service.vue (1)
9-9: Consider internationalization for the title text.While the implementation is correct for a demo, consider using an i18n system for the title text "选择权签人" to support multiple languages in a production environment.
Example implementation using Vue i18n:
- title="选择权签人" + :title="$t('hrapprover.selectApprover')"packages/theme/src/dept/index.less (1)
30-34: Consider responsive design for text widthThe fixed width of 80px for text and label elements might cause issues on different screen sizes or with longer text content (though mitigated by text-overflow: ellipsis).
Consider using:
- A relative width unit or min/max width constraints
- A responsive approach using media queries for different screen sizes
- width: 80px; + min-width: 80px; + max-width: 120px; + width: fit-content;packages/theme/src/base/reset.less (1)
180-193: Consider using CSS variables for margin values.The implementation looks good! However, for better maintainability, consider extracting the margin value to a CSS variable.
+:root { + --tv-popper-margin: 4px; +} .tiny-popper { &[x-placement^='top'] { - margin-bottom: 4px; + margin-bottom: var(--tv-popper-margin); } &[x-placement^='bottom'] { - margin-top: 4px; + margin-top: var(--tv-popper-margin); } &[x-placement^='right'] { - margin-left: 4px; + margin-left: var(--tv-popper-margin); } &[x-placement^='left'] { - margin-right: 4px; + margin-right: var(--tv-popper-margin); } }packages/vue/src/dept/src/pc.vue (1)
21-21: Consider a more responsive approach for dialog positioning.While setting
top="10vh"provides consistent positioning, it might not be optimal for all viewport sizes. On smaller screens, this fixed position could make the dialog appear too close to the top.Consider these alternatives:
- top="10vh" + :style="{ top: Math.max(window.innerHeight * 0.1, 50) + 'px' }"Or use CSS custom properties for better maintainability:
- top="10vh" + class="tiny-dept-dialog"With corresponding CSS:
.tiny-dept-dialog { --min-top-spacing: 50px; --preferred-top-ratio: 10vh; top: max(var(--min-top-spacing), var(--preferred-top-ratio)); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (10)
examples/sites/demos/pc/app/hrapprover/custom-service-composition-api.vue(1 hunks)examples/sites/demos/pc/app/hrapprover/custom-service.vue(1 hunks)packages/theme/src/base/reset.less(1 hunks)packages/theme/src/dept/index.less(2 hunks)packages/theme/src/dept/vars.less(1 hunks)packages/theme/src/espace/index.less(1 hunks)packages/theme/src/roles/index.less(1 hunks)packages/theme/src/roles/vars.less(0 hunks)packages/vue/src/dept/package.json(1 hunks)packages/vue/src/dept/src/pc.vue(1 hunks)
💤 Files with no reviewable changes (1)
- packages/theme/src/roles/vars.less
✅ Files skipped from review due to trivial changes (1)
- packages/vue/src/dept/package.json
🔇 Additional comments (10)
packages/theme/src/dept/vars.less (2)
15-16: Verify accessibility impact of reduced label prominence.
The changes to make labels less prominent (secondary color and regular weight) look good from a visual hierarchy perspective. However, please ensure these changes maintain sufficient contrast ratios for accessibility.
✅ Verification successful
Based on the search results, I can now provide a conclusive response:
The color change to secondary text is consistent with the design system and maintains accessibility.
The search results show that --tv-color-text-secondary (value: #595959) is widely used across the component library for secondary text elements, labels, and descriptions. This color is part of the base theme variables and follows a consistent pattern where:
- Primary text is used for main content
- Secondary text (#595959) is specifically designated for "secondary information" and labels
- The color provides sufficient contrast against light backgrounds
The change aligns with the established design patterns in the codebase where similar components like Tree, NavMenu, and Grid all use this same secondary text color for their labels.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if there are any accessibility-related tests or documentation
# that might need updating with these style changes
# Look for accessibility tests
rg -l "accessibility|a11y|contrast|WCAG" --type md
rg -l "accessibility|a11y|contrast|WCAG" --type vue
Length of output: 318
Script:
#!/bin/bash
# Let's check the actual color values and any existing color contrast guidelines
rg -A 3 "tv-color-text-secondary|tv-color-text-primary" --type less
# Also check for any existing color contrast or accessibility documentation
fd -e md -e mdx . | xargs rg -l "contrast|color|accessibility"
# Check the actual usage of these label styles in components
rg -l "tv-Dept-label" --type vue --type less
Length of output: 13686
15-16: Consider updating style documentation.
Since these changes affect the visual appearance of department labels across the application, consider updating the component's style documentation or design guidelines to reflect these new defaults.
packages/theme/src/espace/index.less (1)
25-25: LGTM! Verify spacing consistency across components.
The increased margin improves visual separation between communication items. This aligns well with modern UI practices for better readability.
Let's verify if this spacing is consistent with other similar components:
✅ Verification successful
Spacing pattern is consistent with component-specific requirements
The 24px margin is appropriately used across different components where larger spacing is needed:
- Vertical spacing for dividers (24px 0)
- Bottom spacing for popeditor (0 auto 24px)
- Horizontal spacing for communication items (0 24px 0 0)
While some components use 8px margins for tighter spacing needs, the 24px spacing in the espace component aligns with other instances where prominent separation is desired.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for other margin definitions in the theme package
# to ensure consistent spacing across components
# Search for margin definitions in theme files
rg -g '*.less' -g '*.css' 'margin:.*24px' packages/theme/
Length of output: 245
Script:
#!/bin/bash
# Let's check for other margin values in these specific components to understand the spacing patterns
rg -g '*.less' -g '*.css' 'margin:.*\d+px' packages/theme/src/espace/ packages/theme/src/popeditor/ packages/theme/src/divider/
# Also check for any spacing variables that might define these values
rg -g '*.less' -g '*.css' '@space|@margin' packages/theme/src/
Length of output: 857
examples/sites/demos/pc/app/hrapprover/custom-service-composition-api.vue (1)
Line range hint 1-45: LGTM! The changes look good overall.
The addition of the title property to the tiny-hrapprover component enhances the UI by providing clear context to users. The implementation is clean and consistent with Vue's template syntax.
examples/sites/demos/pc/app/hrapprover/custom-service.vue (1)
Line range hint 1-57: LGTM! The demo implementation is well-structured.
The component demonstrates proper usage of TinyHrapprover with:
- Clear data binding using v-model
- Mock data service implementation
- Appropriate event handling
- Clean and focused styling
packages/theme/src/roles/index.less (2)
Line range hint 30-57: Verify impact of removed selected state styles.
The removal of .is-selected styles and its related variable --tv-Roles-poplist-item-selected-bg-color could affect the user experience. Please ensure:
- Selected states are still visually distinguishable
- This change is intentional and documented
- No other components rely on these styles
#!/bin/bash
# Description: Check usage of removed styles and variables
# Search for any remaining references to the selected state
rg "is-selected" --type vue --type less
# Search for the removed variable
rg "tv-Roles-poplist-item-selected-bg-color"
# Look for similar selected state patterns in other components
rg "selected.*bg-color" --type less27-27: Verify consistency of popover padding across components.
The padding change from 0 to 8px 0 improves spacing, but let's ensure this aligns with other popovers in the system.
✅ Verification successful
Let's search for padding patterns in popover/popper components to verify consistency.
Padding change is consistent with design patterns
The padding value of 8px 0 aligns with the existing padding patterns across the codebase, particularly in similar components:
- Autocomplete:
padding: 8px 0 - Date panel:
padding: 8px 0 - Grid menu:
padding: 8px 0 - Pager:
padding: 8px 0
This change maintains consistency with the established design system's spacing patterns.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check padding patterns in other popover components
# Look for similar popover/popper class combinations and their padding values
# Search for popover/popper padding definitions
rg -l "\.popover.*\.popper.*padding:" --type less
# Search for specific padding patterns
rg "padding:\s*(8px 0|0);" --type less
Length of output: 1472
Script:
#!/bin/bash
# Search for padding patterns in less files, focusing on popover/popper related styles
rg "\.popover|\.popper" -A 5 -B 5 --type less | grep -E "padding:|margin:"
# Search specifically for padding: 8px patterns
rg "padding:\s*8px" --type less
Length of output: 1761
packages/theme/src/dept/index.less (1)
25-25: Verify spacing alignment with design system
The margin-bottom has been doubled from 12px to 24px for both search and item elements. While this increased spacing can improve readability, please ensure this aligns with your design system's spacing scale.
Consider using spacing variables from your design system (if available) instead of hard-coded values for better maintainability:
- margin-bottom: 24px;
+ margin-bottom: var(--tv-spacing-xl); // or similar design tokenAlso applies to: 53-53
packages/theme/src/base/reset.less (1)
180-193: Verify the margin value consistency across popper components.
There seems to be an inconsistency between margin values:
.tiny-popperuses 4px margins.tiny-popconfirm-popoverand.tiny-tooltip__popperuse 12px margins
Please verify if this difference is intentional. If not, consider standardizing the margin values across all popper components.
Also applies to: 196-251
packages/vue/src/dept/src/pc.vue (1)
Line range hint 1-180: Implementation follows Vue.js best practices.
The component is well-structured with:
- Proper component registration
- Clear prop and emit definitions
- Consistent use of the tiny-vue design system
- TypeScript integration
packages/vue/src/dept/src/pc.vue
Outdated
| @@ -18,6 +18,7 @@ | |||
| @update:visible="state.open = $event" | |||
| :close-on-click-modal="false" | |||
| width="550px" | |||
| top="10vh" | |||
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (7)
examples/sites/demos/pc/app/hrapprover/basic-usage-composition-api.vue (2)
5-5: Consider internationalizing the title text.While adding a title improves accessibility, consider using an i18n key instead of hardcoding the Chinese text "选择权签人", especially if this component is intended for international usage.
Example modification:
- title="选择权签人" + :title="t('hrapprover.selectRightsSigner')"
Line range hint
1-24: Documentation update needed for the title attribute.Since this is a demo file showcasing the basic usage of the component with Composition API, please update the relevant documentation to explain the purpose and usage of the newly added
titleattribute.Would you like me to help draft the documentation update or create a GitHub issue to track this task?
examples/sites/demos/pc/app/hrapprover/basic-usage.vue (1)
5-5: Consider using i18n for the title text.The hardcoded Chinese text "选择权签人" should ideally be internationalized to support multiple languages and improve maintainability.
Consider using your i18n system, for example:
- title="选择权签人" + :title="t('hrapprover.selectApprover')"examples/sites/demos/pc/app/hrapprover/category-type.vue (1)
6-6: LGTM! Consider implementing i18n for the title.The addition of the title improves UI clarity. However, consider implementing internationalization for better maintainability and accessibility.
Example implementation using Vue i18n:
- title="选择权签人" + :title="$t('hrapprover.selectApprover')"Then in your i18n configuration:
{ "en": { "hrapprover": { "selectApprover": "Select Approval Person" } }, "zh": { "hrapprover": { "selectApprover": "选择权签人" } } }examples/sites/demos/pc/app/hrapprover/disabled-composition-api.vue (1)
7-7: Consider internationalizing the title text.While adding the title improves accessibility, consider using an i18n key instead of hardcoding the Chinese text "选择权签人" to support multiple languages.
- title="选择权签人" + :title="t('hrapprover.approverTitle')"examples/sites/demos/pc/app/hrapprover/disabled.vue (2)
7-7: Consider internationalizing the title text.The title addition improves component clarity, but consider extracting "选择权签人" to a localization file for better maintainability and internationalization support.
6-10: Enhance accessibility with aria-label.Consider adding an
aria-labelattribute to improve accessibility for screen readers, especially since this is a demo showcasing disabled state functionality.<tiny-hrapprover :value="value" :approval-person="person" title="选择权签人" + aria-label="Approval person selector" @update:approvalPerson="person = $event" :disabled="disabled" ></tiny-hrapprover>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
examples/sites/demos/pc/app/hrapprover/basic-usage-composition-api.vue(1 hunks)examples/sites/demos/pc/app/hrapprover/basic-usage.vue(1 hunks)examples/sites/demos/pc/app/hrapprover/category-type-composition-api.vue(1 hunks)examples/sites/demos/pc/app/hrapprover/category-type.vue(1 hunks)examples/sites/demos/pc/app/hrapprover/disabled-composition-api.vue(1 hunks)examples/sites/demos/pc/app/hrapprover/disabled.vue(1 hunks)packages/theme/src/dept/index.less(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- examples/sites/demos/pc/app/hrapprover/category-type-composition-api.vue
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/theme/src/dept/index.less
🔇 Additional comments (1)
examples/sites/demos/pc/app/hrapprover/basic-usage.vue (1)
Line range hint 1-33: LGTM! Clean and well-structured demo implementation.
The component demonstrates proper usage of tiny-hrapprover with clear data bindings and event handling. The styling is appropriately scoped and the implementation is consistent with other demo files.
| @@ -4,6 +4,7 @@ | |||
| <tiny-hrapprover | |||
| :value="value" | |||
| :approval-person="person" | |||
| title="选择权签人" | |||
There was a problem hiding this comment.
💡 Codebase verification
Based on the git history and documentation review, I can now provide a final response:
The title property is a recent addition and documentation needs to be updated
The git history shows that the title property was added in the most recent commit (83a4205) and checking the component's documentation (both Chinese and English versions) reveals that this property is not documented. The documentation should be updated to include:
- The new
titleproperty - Its purpose and usage
- Default value if any
- Type information
🔗 Analysis chain
Verify documentation updates for the new title property.
The title property appears to be a new addition across multiple files. Let's verify if the documentation has been updated accordingly.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if documentation for the title property exists
# Expected: Documentation files containing information about the new title property
# Search in documentation files
rg -g "*.md" "title.*选择权签人|hrapprover.*title" docs/
Length of output: 99
Script:
#!/bin/bash
# Let's first check if there are any documentation directories in the repository
fd -t d "doc|docs"
# Let's also check the root directory for any markdown files that might contain documentation
fd -e md
# Let's also check if this title property is used in other files to understand its context
rg "title=\"选择权签人\""
Length of output: 60663
Script:
#!/bin/bash
# Let's check the documentation for the hrapprover component
cat examples/sites/demos/pc/app/hrapprover/webdoc/hrapprover.cn.md
cat examples/sites/demos/pc/app/hrapprover/webdoc/hrapprover.en.md
# Let's also check if this title property is a new addition by looking at git history
git log -p examples/sites/demos/pc/app/hrapprover/disabled-composition-api.vue
Length of output: 4692
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
Release Notes
New Features
titleattribute with the value "选择权签人" to thetiny-hrapprovercomponent across multiple demo files, enhancing user clarity during the approval process.Style Updates
.tiny-popperclass to improve positioning..item-talk,.item-call, and.item-emailclasses for better item separation.