fix(time-range): [date-picker,time-picker,select,action-menu] optimization demo and modify style for time-range #2481
Conversation
WalkthroughThe pull request introduces several modifications across multiple Vue component files and associated styles. Key changes include updates to text descriptions in the templates, adjustments to bindings for select components, and refinements in CSS styles using variables. Notably, the Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 (
|
|
[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 |
MomoPoppy
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 (5)
examples/sites/demos/pc/app/select/size-composition-api.vue (1)
8-10: Maintain consistent language in labelsWhile the changes correctly demonstrate the default size variant, there's an inconsistency in label language:
- Line 3 uses English ("medium")
- Line 8 uses Chinese ("默认")
Consider using consistent language across all size labels for better readability.
Suggested change:
- <p>默认</p> + <p>default</p>examples/sites/demos/pc/app/select/size.vue (2)
3-3: Maintain consistent language in labelsThe labels mix Chinese ("默认") and English ("medium") terms, which could be confusing for users. Consider using either all Chinese or all English labels across all size examples for better consistency.
- <p>medium</p> + <p>中等</p> - <p>默认</p> + <p>默认大小</p>Also applies to: 8-8
Line range hint
1-22: Consider modernizing the layout structureThe current layout relies heavily on
<br />tags for spacing. Consider using modern CSS layout techniques for better maintainability.<template> - <div> + <div class="select-demo"> - <p>medium</p> - <br /> - <tiny-select v-model="value2" size="medium" multiple :options="options"></tiny-select> - <br /> - <br /> + <div class="select-item"> + <p>medium</p> + <tiny-select v-model="value2" size="medium" multiple :options="options"></tiny-select> + </div> // ... similar changes for other select groups ... </div> </template> <style lang="less" scoped> +.select-demo { + display: flex; + flex-direction: column; + gap: 2rem; +} +.select-item { + display: flex; + flex-direction: column; + gap: 0.5rem; +} .tiny-select { width: 280px; }packages/theme/src/time-range/vars.less (1)
31-35: Improve formatting of multi-line CSS variable declaration.The title padding declaration spans multiple lines in a way that might be hard to maintain. Consider using a single-line format or proper CSS variable concatenation.
--- --tv-TimeRange-title-padding: var(--tv-space-lg, 12px) var(--tv-space-md, 8px) var(--tv-space-md, 8px) --- var(--tv-space-md, 8px); +++ --tv-TimeRange-title-padding: var(--tv-space-lg, 12px) var(--tv-space-md, 8px) var(--tv-space-md, 8px) var(--tv-space-md, 8px);packages/theme/src/time-range/index.less (1)
47-50: Remove unnecessary empty line.The structural changes and use of CSS variables look good, but there's an unnecessary empty line that affects readability.
&__cell { width: 50%; - .tiny-time-panel__content {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
examples/sites/demos/pc/app/action-menu/icon-composition-api.vue(1 hunks)examples/sites/demos/pc/app/action-menu/icon.vue(1 hunks)examples/sites/demos/pc/app/select/size-composition-api.vue(1 hunks)examples/sites/demos/pc/app/select/size.spec.ts(2 hunks)examples/sites/demos/pc/app/select/size.vue(1 hunks)packages/theme/src/time-range/index.less(1 hunks)packages/theme/src/time-range/vars.less(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- examples/sites/demos/pc/app/action-menu/icon.vue
🔇 Additional comments (11)
examples/sites/demos/pc/app/select/size-composition-api.vue (2)
3-5: LGTM! Improved clarity in size demonstration
The changes make the medium size variant more explicit by:
- Using clear English text label
- Adding explicit size attribute
- Maintaining consistent behavior through equivalent v-model binding
3-10: Verify size demonstration consistency across documentation
Let's ensure these changes align with other size demonstration examples in the codebase.
✅ Verification successful
Size demonstration is consistent with component library patterns
The implementation follows the established size patterns across the component library:
- Shows all four standard sizes: medium, default, small, and mini
- Uses consistent size prop values: "medium", "small", "mini" (default without prop)
- Maintains same demonstration order as other form components
- Uses identical Chinese label "默认" for default size
- Follows same styling patterns with width and spacing
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistency in size demonstrations across similar components
# Search for other size demonstration files
echo "Searching for related size demonstration files..."
fd -e vue -e ts --full-path "size.*" examples/sites/demos/pc/app
# Check for size-related text patterns
echo "Checking size label patterns..."
rg -i "medium|default|默认" examples/sites/demos/pc/app --type vue
Length of output: 9662
Script:
#!/bin/bash
# Let's check the specific select component size demonstrations and related components
# Check the select size demonstration files
echo "Checking select size demonstration content:"
cat examples/sites/demos/pc/app/select/size.vue examples/sites/demos/pc/app/select/size-composition-api.vue
# Check similar form components for size patterns
echo -e "\nChecking size patterns in related components:"
for comp in "input" "autocomplete" "base-select"
do
echo -e "\n=== $comp size demonstration ==="
cat "examples/sites/demos/pc/app/$comp/size-composition-api.vue"
done
Length of output: 6921
examples/sites/demos/pc/app/select/size.vue (1)
5-5: LGTM! Verify size demonstration order
The reordering of select components creates a more logical progression of size demonstrations, from medium → default → small → mini.
Let's verify this is consistent across other size demonstration files:
Also applies to: 10-10
packages/theme/src/time-range/vars.less (2)
38-38: Verify the impact of simplified footer padding.
The footer padding has been simplified to a single value. Please ensure this doesn't affect the footer's layout in different scenarios, especially considering the removal of the footer height variable.
#!/bin/bash
# Search for footer styles usage in the component
rg --type=less "tiny-time-range(__footer|\.time-footer)"22-22: Verify the impact of consolidated padding variable.
The consolidation of multiple padding variables into a single --tv-TimeRange-cell-content-padding-bottom might reduce layout flexibility. Please ensure this doesn't adversely affect the component's responsiveness or different use cases.
✅ Verification successful
The previous command had a syntax error. Let's try a broader search to understand the padding usage in TimeRange-related files and identify any potential impact of the consolidation.
Padding consolidation is properly implemented and used consistently
The new --tv-TimeRange-cell-content-padding-bottom variable is correctly defined and used in the component's styles. The search results show:
- The variable is properly used in
index.lessfor cell content padding - It's part of a consistent set of padding variables for different parts of the TimeRange component
- All padding variables follow the same naming convention and use design system spacing tokens
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for removed padding variables usage in the codebase
rg --type=less "tv-TimeRange-(cell-padding|content-padding)"
Length of output: 210
Script:
#!/bin/bash
# Search for TimeRange component styles and usage of padding variables
rg --type=less "TimeRange.*padding|tv-TimeRange.*padding"
# Also check for any Less files that might be importing or using TimeRange styles
fd -e less -x grep -l "TimeRange"
Length of output: 1200
examples/sites/demos/pc/app/action-menu/icon-composition-api.vue (1)
3-3: LGTM! Text description now accurately reflects the implementation.
The updated text "场景1：显示图标" (Scenario 1: Show icons) better describes the actual behavior of the component, which displays both icons and text, rather than the previous text which incorrectly suggested it only showed icons.
examples/sites/demos/pc/app/select/size.spec.ts (2)
8-8: LGTM! Verify selector change aligns with component updates.
The selector change from nth(0) to nth(1) appears correct based on the component modifications mentioned in the summary.
Let's verify the component structure to ensure the selector targets the correct element:
21-21: Verify the medium size height specification.
The medium size height is now explicitly set to 40px, which is significantly larger than the default size (32px). Please verify this is the intended height for medium-sized selects.
Let's check the design specifications and other medium-sized components:
Also applies to: 26-27
✅ Verification successful
Let me analyze the CSS structure and height specifications to verify the medium size height value.
Height specification of 40px for medium-sized select is correct
The height value of 40px for medium-sized select components is consistent across the codebase. Multiple components including select, input, time-picker, cascader, and button all use the same 40px height specification for their medium size variant, as evidenced by their respective test files:
- select:
height: 40px - input:
height: 40px - time-picker:
height: 40px - cascader:
height: 40px - button:
height: 40px
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for other medium-sized component height specifications
# Look for height-related CSS for medium-sized components
rg "tiny-.*-medium.*height:|height:.*40px" --type css
Length of output: 275
Script:
#!/bin/bash
# Let's search for size-related CSS classes and height specifications in the codebase
# First, let's look for select-related CSS files
fd -e css -e scss | xargs rg -l "tiny-select"
# Then, let's look specifically for medium size related styles
rg "medium.*height|height.*medium" --type css -A 2 -B 2
# Also search for size-related constants or configurations
fd "select|size" -e ts -e js | xargs rg "medium.*size|size.*medium|height.*40"
Length of output: 5394
packages/theme/src/time-range/index.less (3)
41-41: LGTM! Good use of CSS variables.
The switch to CSS variables for padding improves maintainability and theming flexibility. The removal of fixed height/line-height constraints (as per the summary) allows for better content adaptability.
57-57: LGTM! Good use of semantic variable naming.
The changes maintain consistency with CSS variables while using semantic naming (e.g., border-color-divider) that clearly indicates the purpose of each style property.
Also applies to: 60-60
Line range hint 1-100: Verify PR title and type.
The PR title contains "dosc" suggesting documentation changes, but this file contains purely styling changes. Consider updating the PR title and type to accurately reflect the nature of the changes.
| --tv-TimeRange-header-line-height: 48px; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use existing height variable instead of fixed value.
Consider using var(--tv-size-height-xl) instead of the hardcoded 48px to maintain consistency with the design system and improve maintainability.
--- --tv-TimeRange-header-line-height: 48px;
+++ --tv-TimeRange-header-line-height: var(--tv-size-height-xl, 48px);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| --tv-TimeRange-header-line-height: 48px; | |
| --tv-TimeRange-header-line-height: var(--tv-size-height-xl, 48px); | |
There was a problem hiding this comment.
这里可以用变量：var(--tv-TimeRange-header-height)
There was a problem hiding this comment.
@zzcr，感谢您的建议。您说得对，应当使用变量 var(--tv-TimeRange-header-height) 来替代硬编码的 48px，以保持与设计系统的一致性。
修改如下：
--- --tv-TimeRange-header-line-height: 48px;
+++ --tv-TimeRange-header-line-height: var(--tv-TimeRange-header-height);✏️ Learnings added
Learnt from: zzcr
PR: opentiny/tiny-vue#2481
File: packages/theme/src/time-range/vars.less:27-28
Timestamp: 2024-11-04T09:35:12.932Z
Learning: 在 `packages/theme/src/time-range/vars.less` 文件中，应使用 `var(--tv-TimeRange-header-height)` 作为 `--tv-TimeRange-header-line-height` 的值，以保持一致性。
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
MomoPoppy
changed the title
|
|
github-actions
bot
removed
the
documentation
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
tiny-selectcomponent functionality with updated bindings and sizes.Bug Fixes
Style