refactor(tag): refactor fix lost css for button,tag #2301
Conversation
WalkthroughThe changes in this pull request involve modifications to the 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
|
[e2e-test-warn] The title of the Pull request should look like "fix(vue-renderless): [action-menu, alert] fix xxx bug". Please make sure you've read our contributing guide |
![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
🧹 Outside diff range and nitpick comments (5)
packages/theme/src/tag/index.less (3)
24-25: LGTM! Consider enhancing the comment.The addition of
height: fit-content;is a good solution to prevent unwanted stretching in flex layouts. Using a CSS variable forline-heightallows for easy theming and consistency.Consider updating the comment to be more specific:
- height: fit-content; // 避免 flex时，高度拉伸 + height: fit-content; // Prevent height stretching when used in flex containersThis change would make the comment more accessible to non-Chinese speakers and explicitly mention flex containers.
26-27: LGTM! Consider enhancing the comment.The addition of
max-width: 200px;is a good constraint to prevent excessively long tags from disrupting layouts.Consider updating the comment to be more specific and in English:
- max-width: 200px; // 新增约束，避免太长的标签 + max-width: 200px; // Constrain width to prevent excessively long tagsThis change would make the comment more accessible to non-Chinese speakers and provide a clearer explanation of the constraint's purpose.
28-30: LGTM! Consider adding a title attribute for accessibility.The combination of
white-space: nowrap;,text-overflow: ellipsis;, andoverflow: hidden;creates a clean, single-line appearance for the tag and handles text overflow effectively.To improve accessibility for tags with truncated text, consider adding a
titleattribute to the tag element in the component's template. This will allow users to see the full text on hover. For example:<template> <div class="@{tag-prefix-cls}" :title="text"> {{ text }} </div> </template>This addition would ensure that users can access the full content of longer tags that are visually truncated.
packages/vue/src/tree/src/tree-node.vue (2)
117-117: LGTM: Improved loading icon implementation.The replacement of the inline SVG with the
<icon-loading>component is a good refactor. It simplifies the code and likely promotes consistency across the application.Consider adding a size prop to the
<icon-loading>component if it's not already present, to ensure it fits well within the tree node structure:<icon-loading v-if="node.loading" class="tiny-tree-node__loading" :size="16" />
Line range hint
311-315: LGTM: New prop for controlling focus background visibility.The addition of the
isShowFocusBgprop is well-implemented. It's properly typed as a Boolean with a sensible default value oftrue.Consider adding a brief JSDoc comment to provide more context about this prop's purpose and usage:
/** * Controls the visibility of the focus background on tree nodes. * When true, a background color is shown on focused nodes. * @default true */ isShowFocusBg: { type: Boolean, default: true }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
- examples/sites/demos/pc/app/tag/size-composition-api.vue (1 hunks)
- examples/sites/demos/pc/app/tag/size.vue (1 hunks)
- packages/theme/src/alert/index.less (1 hunks)
- packages/theme/src/button/index.less (1 hunks)
- packages/theme/src/button/vars.less (1 hunks)
- packages/theme/src/tag/index.less (1 hunks)
- packages/theme/src/tag/vars.less (1 hunks)
- packages/theme/src/tree/index.less (1 hunks)
- packages/vue/src/tree/src/tree-node.vue (3 hunks)
🧰 Additional context used
🔇 Additional comments (13)
examples/sites/demos/pc/app/tag/size-composition-api.vue (2)
Line range hint
1-23: Consider addressing CSS adjustments mentioned in PR objectivesWhile the template changes look good, I noticed that the PR objectives mention several CSS adjustments:
- Adding line-height to buttons
- Preventing the height of tags from stretching
- Fixing the alignment of alert icons
- Upgrading the loading icon for tree nodes
However, this file doesn't contain any CSS changes related to these objectives. It might be worth considering if additional CSS modifications are needed in this file or elsewhere to fully address the PR objectives.
Let's check if these CSS changes have been implemented in other files:
#!/bin/bash # Description: Check for CSS changes related to PR objectives echo "Searching for CSS changes related to button line-height:" rg --type css 'button.*line-height' echo "Searching for CSS changes related to tag height:" rg --type css 'tag.*height' echo "Searching for CSS changes related to alert icon alignment:" rg --type css 'alert.*icon.*align' echo "Searching for CSS changes related to tree node loading icon:" rg --type css 'tree.*loading.*icon'This script will help us identify if the mentioned CSS adjustments have been implemented in other files within the project.
3-6: LGTM: Consistent addition oftype="success"to all tagsThe changes look good. The
type="success"attribute has been added consistently to all<tiny-tag>components, which aligns with the PR objectives of refactoring and CSS adjustments. This modification enhances the visual consistency of the tags across different sizes.However, I have a few points to consider:
The PR objectives mention preventing the height of tags from stretching. It's not clear if this change alone achieves that goal. You might want to verify if additional CSS changes are needed.
The PR summary doesn't explicitly state the reason for adding the
type="success"to all tags. It might be helpful to clarify the motivation behind this change in the PR description.To ensure consistency across the codebase, let's check if similar changes have been made in other files:
This script will help us identify if there are any inconsistencies in the usage of
type="success"for<tiny-tag>components across the project.examples/sites/demos/pc/app/tag/size.vue (5)
3-3: LGTM: Consistent type addition across all tags.The addition of
type="success"to the medium-sized tag enhances its visual appearance and maintains consistency with the other tags in the demo.
4-4: LGTM: Consistent type addition.The addition of
type="success"to the default-sized tag is consistent with the changes made to other tags, maintaining uniformity in the demo.
5-5: LGTM: Consistent type addition.The addition of
type="success"to the small-sized tag maintains consistency with the other tags in the demo.
6-6: LGTM: Consistent type addition completes the demo refactor.The addition of
type="success"to the mini-sized tag completes the consistent application of the success type across all tag sizes in the demo. This change, along with the previous ones, successfully refactors the tag demo to showcase a uniform appearance while still demonstrating different sizes.These changes align well with the PR objectives of refactoring and fixing CSS-related issues. The consistent application of
type="success"across all tag sizes will likely improve the visual coherence of the demo.
Line range hint
1-30: Overall assessment: Successful refactor of tag demoThe changes in this file consistently add
type="success"to all<tiny-tag>components across different sizes. This refactor enhances the visual consistency of the demo while still effectively showcasing the various tag sizes. The modifications align well with the PR objectives of refactoring and addressing CSS-related issues.No changes were made to the script or style sections, indicating that the functionality and custom styling remain unchanged. This focused approach to updating only the template section suggests a clean and targeted refactor.
packages/theme/src/alert/index.less (1)
39-39: LGTM: Icon alignment adjustmentThe addition of
margin-top: 2px;to the alert icon class aligns with the PR objective of fixing alert icon alignment. This small adjustment should help in fine-tuning the vertical positioning of the icon within the alert component.To ensure this change achieves the desired visual effect across different alert types and sizes, please verify the alignment in various scenarios. Consider running the following commands to check for any potential regressions:
✅ Verification successful
Verified: Icon alignment adjustment confirmed
The
margin-top: 2px;addition to the alert icon class is the only margin-top adjustment related to icons in the codebase. This confirms that the change aligns with the PR objective of fixing alert icon alignment without introducing other margin-top conflicts.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any other icon alignment adjustments in the codebase # Test 1: Search for other margin-top adjustments to icons echo "Searching for other margin-top adjustments to icons:" rg --type less "margin-top.*icon" # Test 2: Check for any icon-related CSS changes in alert components echo "Checking for icon-related CSS changes in alert components:" rg --type less "icon" packages/theme/src/alertLength of output: 3860
packages/theme/src/tag/vars.less (1)
24-24: LGTM: Subtle adjustment to tag vertical paddingThe changes to
--tv-Tag-padding-y-miniand--tv-Tag-padding-yfrom 0 to 0.5px are appropriate. This subtle adjustment:
- Addresses the PR objective of preventing tags from stretching in height.
- Maintains consistency across mini and default tag sizes.
- Provides a minimal visual enhancement without causing significant layout changes.
The use of CSS variables allows for easy overrides if needed in specific use cases.
Also applies to: 40-40
packages/theme/src/button/index.less (2)
25-25: Improved button text alignment with line-height propertyThe addition of
line-height: var(--tv-Button-line-height);to the base button class is a good improvement. This change will help ensure consistent vertical alignment of text within buttons across different sizes and themes.Benefits:
- Enhances text alignment within buttons
- Uses a CSS variable for easy customization and theme consistency
- Aligns with the PR objectives of refactoring and fixing CSS issues
The placement and implementation of this property are appropriate and don't conflict with existing styles.
Line range hint
1-383: Overall assessment of button stylingThe changes made to this file are minimal and focused, addressing the PR objectives of refactoring and fixing CSS issues. The addition of the
line-heightproperty enhances button text alignment without disrupting the existing comprehensive styling structure for various button states, sizes, and themes.The file maintains a well-organized approach to button styling, using LESS mixins and CSS variables effectively. This change integrates seamlessly with the existing code and should improve the overall appearance of buttons in the UI.
No further modifications are necessary as a result of this change, and the risk of unintended side effects is minimal.
packages/theme/src/tree/index.less (1)
432-435: Simplified loading indicator stylesThe
.tree-node__loadingclass has been significantly simplified. This change suggests a shift in the loading indicator's implementation:
- The removal of circular-specific styles and animations indicates a possible change in the visual representation of the loading state.
- Retaining the margin properties ensures consistent positioning of the loading indicator.
To ensure this change doesn't negatively impact the user experience, please verify:
- The new loading indicator is visually appropriate and provides clear feedback to users.
- The change is consistent across all instances where the tree loading state is used.
If you need help implementing a new loading indicator or adjusting the styles to maintain visual consistency, please let me know.
✅ Verification successful
Loading indicator styles verified
The changes to the
.tree-node__loadingclass have been confirmed as consistent across the codebase. All circular-specific styles have been successfully removed, ensuring a uniform loading indicator implementation.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other occurrences of tree-node__loading to ensure consistency rg --type less "tree-node__loading" packages/theme/srcLength of output: 54
Script:
#!/bin/bash # Re-run search for occurrences of 'tree-node__loading' in LESS files to ensure consistency rg --type=less "tree-node__loading" packages/theme/srcLength of output: 54
Script:
#!/bin/bash # Search for occurrences of 'tree-node__loading.circular' in LESS files to verify removal of circular-specific styles rg --type=less "\.tree-node__loading\.circular" packages/theme/srcLength of output: 68
packages/vue/src/tree/src/tree-node.vue (1)
Line range hint
27-27: LGTM: New class binding for focus background.The addition of the
'is-show-focus-bg': isShowFocusBgclass binding is a good implementation. It allows for flexible control over the focus background visibility, which is consistent with the new prop added to the component.
| @@ -242,7 +240,7 @@ import { renderless, api } from '@opentiny/vue-renderless/tree-node/vue' | |||
| import CollapseTransition from '@opentiny/vue-collapse-transition' | |||
| import { | |||
| iconChevronRight, | |||
| iconLoading, | |||
| iconLoadingShadow, | |||
There was a problem hiding this comment.
💡 Codebase verification
Issues Found: Inconsistent usage of iconLoading detected across the codebase.
Several .vue files still use iconLoading instead of iconLoadingShadow. This inconsistency may lead to visual discrepancies and unexpected behavior.
Affected files include, but are not limited to:
packages/vue/src/select/src/mobile-first.vuepackages/vue/src/modal/src/pc.vueexamples/sites/demos/pc/app/time-line/custom-icon.vueexamples/sites/demos/mobile/app/loading/spinner.vue- ... [additional files as per the script output]
Please ensure all instances of iconLoading are updated to iconLoadingShadow to maintain consistency.
🔗 Analysis chain
LGTM: Updated loading icon import and registration.
The change from iconLoading to iconLoadingShadow is consistent with the earlier template modification. This update likely introduces a new visual style for the loading icon.
To ensure this change doesn't introduce any unexpected visual regressions, please run the following verification script:
This script will help identify any inconsistencies in the usage of the new iconLoadingShadow across the codebase.
Also applies to: 328-328
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of iconLoadingShadow across the codebase
# Test: Check for any remaining usage of the old iconLoading
echo "Checking for any remaining usage of iconLoading:"
rg "iconLoading[^Shadow]" --type vue
# Test: Verify consistent usage of iconLoadingShadow
echo "Verifying consistent usage of iconLoadingShadow:"
rg "iconLoadingShadow" --type vue
# Test: Check for any potential naming conflicts
echo "Checking for potential naming conflicts:"
rg "IconLoading" --type vue
Length of output: 489
Script:
#!/bin/bash
# Description: Verify the usage of iconLoadingShadow across the codebase
# Test: Check for any remaining usage of the old iconLoading
echo "Checking for any remaining usage of iconLoading:"
rg "iconLoading[^Shadow]" -g "*.vue"
# Test: Verify consistent usage of iconLoadingShadow
echo "Verifying consistent usage of iconLoadingShadow:"
rg "iconLoadingShadow" -g "*.vue"
# Test: Check for any potential naming conflicts
echo "Checking for potential naming conflicts:"
rg "IconLoading" -g "*.vue"
Length of output: 7742
| // 按钮的行产， | ||
| --tv-Button-line-height: var(--tv-border-width); |
There was a problem hiding this comment.
💡 Codebase verification
Potential Issue: Line Height Set to Border Width
The variable --tv-Button-line-height is currently set to var(--tv-border-width) (1px). Assigning a border width value to line height is unconventional and may result in text alignment issues within buttons.
Consider the following:
- Intentional Design? If this assignment is deliberate for a specific design effect, please provide the rationale to ensure it aligns with the overall UI/UX goals.
- Potential Styling Conflicts: Using border width for line height might cause cramped text or inconsistent button heights across different contexts.
- Variable Separation: It may be beneficial to define a separate variable for line height to maintain clarity and flexibility in styling.
🔗 Analysis chain
Clarify the purpose of setting line height to border width
The addition of --tv-Button-line-height is consistent with the existing naming conventions. However, setting the line height to the border width (var(--tv-border-width)) is unusual and might lead to unexpected button appearances. This could affect text alignment and overall button height.
Consider the following:
- Is this intentional? If so, please explain the reasoning behind this decision.
- Have you tested this change with various button sizes and content to ensure it doesn't cause layout issues?
- The comment above the variable is in Chinese. For consistency and maintainability, consider translating it to English if that's the primary language used in the project.
To assess the impact of this change, let's check how the --tv-border-width variable is defined and used:
This will help us understand the context and potential implications of using border width as line height.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the definition and usage of --tv-border-width
rg --type less '--tv-border-width'
Length of output: 454
Script:
#!/bin/bash
# Search for the definition and usage of --tv-border-width using the correct rg syntax
rg --type less -e '--tv-border-width'
Length of output: 1442
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
button增加 Line-height
防止tag的高度拉伸
修复alert-icon的对齐
升级tree-node的加载中图标
Summary by CodeRabbit
New Features
typeattribute to<tiny-tag>components for improved tagging.Enhancements
Bug Fixes