fix(transfer): [transfer] update smb design #2249
Conversation
WalkthroughThe pull request includes modifications to various Vue component files and their associated test cases. Key changes involve the removal of specific CSS assertions in tests, updates to the styling of Cascader components, and adjustments to CSS variables affecting visual presentation. Notably, a new property for displaying lines in tree components is introduced, and several CSS properties are refined across different components to enhance layout and styling consistency. Changes
Possibly related PRs
Suggested labels
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 (
|
shenjunjian
changed the title
![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
🧹 Outside diff range and nitpick comments (7)
examples/sites/demos/pc/app/transfer/nested-tree-composition-api.vue (1)
77-77: LGTM! Consider reordering properties for consistency.The addition of
showLine: trueto thetreeConfigobject is a good enhancement. It will improve the visual representation of the tree structure in the transfer component, making it easier for users to understand the hierarchy of items.For consistency with the object's current structure, consider moving the
showLineproperty aftercheckStrictly:const treeConfig = ref({ - showLine: true, showCheckbox: true, nodeKey: 'id', checkStrictly: true, + showLine: true, filterNodeMethod(query, data) { return data.label.includes(query) } })This ordering aligns better with the existing property arrangement, placing boolean configurations together.
examples/sites/demos/pc/app/transfer/nested-tree.vue (1)
Line range hint
1-93: Suggestions for improving component readability and maintainabilityWhile the current implementation is functional, consider the following improvements:
- Rename the
valuedata property to something more descriptive, such asselectedNodesortransferredItems.- Add comments to explain the purpose of the component and how to use it, especially regarding the tree structure and transfer functionality.
- Implement error handling or validation for the tree data structure to ensure it always has the expected format.
Example of adding comments:
<script lang="jsx"> import { Transfer, Tree } from '@opentiny/vue' /** * NestedTreeTransfer Component * * This component provides a transfer interface with a nested tree structure. * It allows users to move items between two lists, where items are organized in a hierarchical manner. * * Usage: * <nested-tree-transfer v-model="selectedItems" :data="treeData" /> */ export default { // ... rest of the component code } </script>These changes would enhance the component's maintainability and make it easier for other developers to understand and use.
packages/theme/src/cascader-node/vars.less (1)
Line range hint
1-51: Summary: Verify cascader styling changes in the context of the entire componentThe changes to the cascader node styling variables appear to be part of a design update, shifting towards a more subtle visual presentation. While the modifications themselves don't present any syntax issues, it's crucial to verify their impact on the overall user experience of the cascader component.
Consider the following recommendations:
- Review these changes in the context of the entire cascader component to ensure they align with the intended design direction.
- Conduct thorough testing, including accessibility and usability tests, to validate that the new styles maintain or improve the component's effectiveness.
- If not already done, update the component's documentation to reflect these styling changes and any new best practices for using the cascader in different contexts.
To assist in this verification, you can run the following command to locate relevant component files and tests:
#!/bin/bash # Find cascader component files and tests echo "Cascader Component Files:" fd -e vue -e js -e ts cascader packages/ echo "\nCascader Test Files:" fd -e spec.js -e spec.ts -e test.js -e test.ts cascader packages/examples/sites/demos/pc/app/cascader/filter-mode.vue (1)
12-12: Consider using a CSS class for spacing instead of inline style.The addition of vertical spacing between the Cascader components improves the layout. However, I have a few suggestions to enhance this change:
- Instead of using an inline style, consider moving this to a CSS class. This approach improves maintainability and consistency across the application.
- Verify if the 16px spacing aligns with your project's design system. If you have predefined spacing values, it would be better to use one of those for consistency.
Here's an example of how you could refactor this:
- <div style="margin-top: 16px"> + <div class="cascader-spacing"> <tiny-cascader v-model="value" :options="options" shape="filter" label="开发文档" blank></tiny-cascader> </div>Then add a style block to your component (or in a separate CSS file if you prefer):
<style scoped> .cascader-spacing { margin-top: var(--tiny-spacing-medium, 16px); /* Adjust the variable name as per your design system */ } </style>This approach allows for easier maintenance and better alignment with your design system.
packages/vue/src/transfer-panel/src/pc.vue (2)
73-76: LGTM! Consider using CSS classes for styling consistency.The changes to the icon classes improve clarity and the styling adjustment for the unchecked state is appropriate. However, for better maintainability and consistency, consider moving the inline style to a CSS class.
You could refactor the inline style as follows:
-<icon-check - v-if="!(state.checked.length > 0 && state.checked.indexOf(item[state.keyProp]) > -1)" - class="tiny-svg-size icon-no-checked" - style="fill: transparent" -/> +<icon-check + v-if="!(state.checked.length > 0 && state.checked.indexOf(item[state.keyProp]) > -1)" + class="tiny-svg-size icon-no-checked transparent-fill" +/>Then add the following CSS class:
.transparent-fill { fill: transparent; }This approach would make it easier to manage styles across the component.
176-177: LGTM! Consider enhancing accessibility for icon states.The update to use
iconYes()for bothIconCheckedSurandIconCheckcomponents is consistent with the import changes. This simplification can lead to more maintainable code.To enhance accessibility and user experience, consider adding aria labels or titles to differentiate between the checked and unchecked states:
-IconCheckedSur: iconYes(), -IconCheck: iconYes(), +IconCheckedSur: { + ...iconYes(), + props: { + ...iconYes().props, + title: 'Checked' + } +}, +IconCheck: { + ...iconYes(), + props: { + ...iconYes().props, + title: 'Unchecked' + } +},This approach would provide additional context for screen readers and improve the overall accessibility of the component.
packages/theme/src/transfer/index.less (1)
115-116: LGTM! Consider translating the comment to English.The changes improve the vertical alignment of text and icons within the transfer buttons. The
transform: translateY(-3px)adjusts the position, whiledisplay: inline-blockensures proper inline behavior.Consider translating the Chinese comment to English for consistency and to improve maintainability for non-Chinese speaking developers. For example:
- transform: translateY(-3px); // 适配 xdesign, 让span居中 + transform: translateY(-3px); // Adapt for xdesign, center the span element display: inline-block;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (11)
- examples/sites/demos/pc/app/cascader/check-strictly-multiple.spec.ts (0 hunks)
- examples/sites/demos/pc/app/cascader/filter-mode-composition-api.vue (1 hunks)
- examples/sites/demos/pc/app/cascader/filter-mode.vue (1 hunks)
- examples/sites/demos/pc/app/cascader/size.spec.ts (1 hunks)
- examples/sites/demos/pc/app/transfer/nested-tree-composition-api.vue (1 hunks)
- examples/sites/demos/pc/app/transfer/nested-tree.vue (1 hunks)
- packages/theme/src/cascader-node/vars.less (2 hunks)
- packages/theme/src/filter-box/index.less (1 hunks)
- packages/theme/src/filter-box/vars.less (1 hunks)
- packages/theme/src/transfer/index.less (1 hunks)
- packages/vue/src/transfer-panel/src/pc.vue (3 hunks)
💤 Files with no reviewable changes (1)
- examples/sites/demos/pc/app/cascader/check-strictly-multiple.spec.ts
✅ Files skipped from review due to trivial changes (1)
- examples/sites/demos/pc/app/cascader/filter-mode-composition-api.vue
🧰 Additional context used
🔇 Additional comments (6)
examples/sites/demos/pc/app/cascader/size.spec.ts (1)
9-9: Verify the intentional change in small-sized Cascader input height.The expected height for the small-sized Cascader input has been updated from 32px to 28px. This change appears to maintain a logical size progression (24px for mini, 28px for small, 40px for medium).
Please ensure that:
- This change aligns with the updated design specifications for the Cascader component.
- The corresponding CSS in the component implementation has been updated to match this new expectation.
- Other components using the 'small' size variant have been consistently updated if necessary.
To verify the consistency of this change across the codebase, please run the following script:
Consider adding a comment explaining the reason for this height change to improve maintainability.
packages/theme/src/filter-box/vars.less (1)
26-26: LGTM. Verify consistency and update documentation if needed.The change from
--ti-common-font-size-baseto--ti-common-font-size-0looks good and seems to be part of a standardization effort. Both variables have the same fallback value of 12px, which should maintain the current visual appearance if the new variable is not defined.To ensure consistency across the project, please run the following script:
If this change is part of a larger update to the design system or variable naming convention, consider updating the relevant documentation to reflect this change.
packages/theme/src/cascader-node/vars.less (2)
51-51: Verify the impact of changing active item font weightThe font weight for active (highlighted) items in the cascader dropdown has been changed from bold to normal. While this change might be part of a broader design update for a more subtle look, it's crucial to ensure it doesn't negatively impact the user experience.
Could you please provide the reasoning behind this change? It would be beneficial to verify that this modification:
- Maintains clear visual distinction for active items.
- Doesn't impair usability, especially for users who rely on strong visual cues.
To assist in verifying the impact, you can run the following command to check for any related usability or accessibility tests:
#!/bin/bash # Search for relevant test files fd -e spec.js -e spec.ts -e test.js -e test.ts | xargs rg -i "cascader.*active item|cascader.*highlighted item|cascader.*font weight"
23-23: Clarify the rationale for changing the selectable text colorThe text color for selectable items in the cascader dropdown has been changed from a highlighted state to the primary text color. While this change might align with updated design guidelines or improve consistency, it's important to ensure it doesn't negatively impact the user experience or accessibility.
Could you please clarify the reasoning behind this change? Additionally, it would be beneficial to verify that this modification:
- Maintains sufficient contrast for accessibility standards.
- Doesn't reduce the visual clarity of selectable items for users.
To assist in verifying the impact, you can run the following command to check for any related accessibility or UX tests:
packages/theme/src/filter-box/index.less (1)
90-90: Consider using a relative unit for line-heightThe change from
inheritto a fixed value of1.5forline-heightmay improve readability. However, it's important to verify that this doesn't cause any layout issues within the filter box, especially considering the fixed height and flexbox alignment of the parent container.To maintain flexibility and scalability, consider using a relative unit like
eminstead of a unitless value. For example:- line-height: 1.5; + line-height: 1.5em;This allows the line height to scale with the font size while maintaining the desired spacing.
Please verify the visual appearance of the filter box component across different contexts and screen sizes to ensure this change doesn't negatively impact the layout or alignment.
packages/vue/src/transfer-panel/src/pc.vue (1)
133-133: LGTM! Verify icon suitability for both states.The consolidation of icon imports to use
iconYesfor both checked and unchecked states is a good optimization. However, it's crucial to ensure that this icon is visually appropriate for both states.Please confirm that the
iconYesicon is visually suitable for representing both the checked and unchecked states in the transfer panel. If possible, provide a visual comparison of the previous icons (iconCheckedSurandiconCheck) with the newiconYesicon to ensure there's no loss in visual clarity or user experience.
| @@ -79,6 +79,7 @@ export default { | |||
| } | |||
| ], | |||
| treeConfig: { | |||
| showLine: true, | |||
There was a problem hiding this comment.
💡 Codebase verification
LGTM: Addition of showLine property enhances tree visualization.
While showLine: true is consistently used across other components, it is not currently documented. Please update the Tree component documentation to include the showLine property to ensure clarity and maintain comprehensive documentation.
🔗 Analysis chain
LGTM: Addition of showLine property enhances tree visualization.
The addition of showLine: true to the treeConfig object is a valid enhancement. This property will enable the display of lines connecting tree nodes, improving the visual representation of the nested structure.
To ensure this change is consistent with the component's documentation and usage, let's verify:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the Tree component documentation mentions the showLine property
# and if it's used consistently in other examples.
# Search for Tree component documentation
echo "Searching for Tree component documentation:"
rg --type=vue --type=md "Tree.*component" -g '!node_modules'
# Search for other usage of showLine property in Tree configurations
echo "\nSearching for other usage of showLine property:"
rg --type=vue "showLine:\s*(true|false)" -g '!node_modules'
Length of output: 386
Script:
#!/bin/bash
# Description: Check if the Tree component documentation mentions the showLine property
# and if it's used consistently in other examples.
# Search for Tree component documentation in .md and .vue files
echo "Searching for Tree component documentation:"
rg "Tree.*component" -g '*.vue' -g '*.md' -g '!node_modules'
# Search for other usage of showLine property in Tree configurations within .vue files
echo "\nSearching for other usage of showLine property:"
rg "showLine:\s*(true|false)" -g '*.vue' -g '!node_modules'
Length of output: 2073
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
对接 smb design 规范
Summary by CodeRabbit