fix(date-picker): [date-picker,select] fix radius bugs for month-table #2442
Conversation
WalkthroughThe changes in this pull request involve updates to various styles and functionalities across multiple components. 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 (
|
|
|
|
![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 (3)
packages/theme/src/month-table/vars.less (1)
37-41: LGTM! The new border-radius variables are well-defined.The new CSS custom properties are properly named, documented, and maintain consistency with existing variables.
Consider adjusting the line breaks for consistency with other similar properties:
- --tv-MonthTable-cell-border-radius-first: var(--tv-border-radius-round, 999px) 0 0 - var(--tv-border-radius-round, 999px); + --tv-MonthTable-cell-border-radius-first: var(--tv-border-radius-round, 999px) 0 0 var(--tv-border-radius-round, 999px);packages/theme/src/month-table/index.less (1)
96-113: LGTM! Consider adding RTL support.The border-radius implementation for first and last rows looks correct and aligns with the PR objective. However, consider adding RTL support for the padding adjustments.
Add RTL support by using logical properties:
&.end-date { div { border-radius: var(--tv-MonthTable-cell-border-radius); - padding-left: 0; + padding-inline-start: 0; } }packages/theme/src/select/index.less (1)
Line range hint
368-370: Consider using a CSS custom property for tooltip max-width.While setting a max-width for tooltips is good practice, consider making this value configurable via a CSS custom property for better theme customization.
- max-width: 300px; + max-width: var(--tv-Select-tooltip-max-width, 300px);Don't forget to add the new variable to
vars.less:--tv-Select-tooltip-max-width: 300px;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- packages/theme/src/month-table/index.less (1 hunks)
- packages/theme/src/month-table/vars.less (1 hunks)
- packages/theme/src/select/index.less (1 hunks)
- packages/vue/src/select/src/pc.vue (1 hunks)
🔇 Additional comments (5)
packages/theme/src/month-table/vars.less (1)
37-41: Verify the usage of new border-radius variables.Let's ensure these new variables are properly utilized in the corresponding Less files.
✅ Verification successful
Border radius variables are properly implemented and used
The variables are correctly defined in
vars.lessand properly utilized inindex.lessfor styling the first and last cells of the month table rows.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if the new border-radius variables are used in the month-table styles # Test: Search for the usage of new border-radius variables rg --type less "tv-MonthTable-cell-border-radius-(first|last)"Length of output: 580
packages/theme/src/month-table/index.less (1)
98-98: Verify the new CSS custom properties.The implementation uses two new custom properties:
--tv-MonthTable-cell-border-radius-firstand--tv-MonthTable-cell-border-radius-last.Let's verify these are properly defined:
Also applies to: 111-111
✅ Verification successful
Custom properties are properly defined and used correctly
The verification shows that both custom properties
--tv-MonthTable-cell-border-radius-firstand--tv-MonthTable-cell-border-radius-lastare:
- Properly defined in
packages/theme/src/month-table/vars.less- Used correctly in
packages/theme/src/month-table/index.less- Follow the fallback pattern using
--tv-border-radius-roundwith a default value🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if the new CSS custom properties are defined in vars.less rg --type less "tv-MonthTable-cell-border-radius-(first|last)" packages/theme/src/Length of output: 600
packages/theme/src/select/index.less (2)
Line range hint
348-366: LGTM! Improved disabled state handling.The changes properly handle disabled states by:
- Setting consistent cursor styles
- Adjusting padding for better visual appearance
- Ensuring proper suffix display
344-347: Verify the visual alignment after margin removal.The removal of right margin from SVG icons in the click-expand state could affect the spacing between the icon and adjacent elements. Please ensure this doesn't create any visual alignment issues, especially in different states (hover, focus, etc.).
✅ Verification successful
Based on the search results, I can now provide a final response:
SVG margin removal is consistent with component's design
The margin removal for
.tiny-svgwithin the.is-click-expandstate is intentional and correct. The codebase shows that:
- The select component has specific handling for SVG icons in different states
- In the click-expand state, the component uses absolute positioning for tags and requires precise control over spacing
- Other SVG margins in the select and select-dropdown components are explicitly defined where needed (e.g.,
margin: var(--tv-Select-collapse-margin)in normal state)🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any related margin or spacing styles for SVG icons rg -l "tiny-svg.*margin|margin.*tiny-svg" --type lessLength of output: 55
Script:
#!/bin/bash # Let's try a broader search for SVG-related styles in the select component rg "tiny-svg" --type less -A 2 -B 2Length of output: 18665
Script:
#!/bin/bash # Search for any margin-related styles within the select component fd "select.*\.less$" --exec rg "margin" {} -A 2 -B 2Length of output: 54
Script:
#!/bin/bash # Look for the specific class context rg "is-click-expand" --type less -A 5 -B 5Length of output: 2536
packages/vue/src/select/src/pc.vue (1)
159-159: Verify the usage of theonly-iconprop in<tiny-tag>Please confirm that the
only-iconprop is supported by the<tiny-tag>component and is implemented correctly.Run the following script to verify if the
only-iconprop is defined in the<tiny-tag>component's props:✅ Verification successful
Based on the code analysis, I can now provide the final response:
The
only-iconprop is properly supported by thetiny-tagcomponentThe verification shows that the
only-iconprop is correctly defined in the tag component's props list (line 41 in packages/vue/src/tag/src/pc.vue) and is properly handled in the component's rendering logic to apply the CSS classtiny-tag--only-iconwhen enabled (line 89).🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if 'only-icon' is a valid prop in 'tiny-tag' component. # Expected result: The 'only-icon' prop should be declared in the props of 'tiny-tag'. # Search for the 'only-icon' prop declaration in 'tiny-tag' component files. fd --type f --name '*.vue' | xargs rg -A 5 -F "export default" | rg -A 10 "tiny-tag" | rg -A 5 "props" | rg "only-icon"Length of output: 366
Script:
#!/bin/bash # Let's first find the tiny-tag component file fd tiny-tag # Then let's search for the props definition in the tiny-tag component rg -A 10 "props:\s*{" --type vue # Also search for any TypeScript interface or type definitions for tiny-tag props rg -A 10 "interface.*TagProps|type.*TagProps" --type typescriptLength of output: 233
Script:
#!/bin/bash # First, let's find the tiny-tag component file fd "tiny-tag" # Then let's search for props definition with fixed regex rg -A 10 "props: {" # Also search for any files containing tiny-tag to understand its implementation rg -l "tiny-tag"Length of output: 7260
Script:
#!/bin/bash # Let's examine the tag component's source code cat packages/vue/src/tag/src/pc.vue # And also check the select component where only-icon is being used cat packages/vue/src/select/src/pc.vueLength of output: 31933
* fix(build): fix build error * fix(docs): 增加上传文件类型说明 (#2439) * fix(tag): update tag's size when only icon, update one place of button's icon size (#2440) * fix(date-picker): [date-picker,select] fix radius bugs for month-table (#2442) * docs(site): fix demos and test e2e (#2444) * test(site): fix icon not correct test e2e (#2445) --------- Co-authored-by: chenxi-20 <[email protected]> Co-authored-by: 申君健 <[email protected]> Co-authored-by: MomoPoppy <[email protected]> Co-authored-by: gimmyhehe <[email protected]>
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
Bug Fixes
Style