-
Notifications
You must be signed in to change notification settings - Fork 278
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(statistic): [statistic] synchronize the statistic component of the release-3.18 version to the dev branch #2717
Conversation
WalkthroughThis pull request introduces modifications to the Changes
Possibly related PRs
Suggested reviewers
Poem
📜 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 (
|
WalkthroughThis PR synchronizes the statistic component from the release-3.18 version to the dev branch, focusing on bug fixes. Key changes include updating the Changes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (3)
examples/sites/demos/pc/app/statistic/basic-usage.vue (1)
11-13
: Clarify default precision for user experience.
By omittingprecision
here, the statistic will fall back to the default. If the intended precision is zero or a specific setting, explicitly passing it ensures clarity.packages/renderless/src/statistic/index.ts (2)
22-25
: Validate the negative precision scenario.
The check(props.precision >= 0)
implies negative precision is not supported. If negative precision is supplied by mistake, ensure it fails gracefully or is set to zero to avoid partial decimal slicing.
26-26
: Safeguard consistency whendisplayValue
is empty.
Ifprops.value
is''
or invalid, you setinteger
to'0'
. This is reasonable but might cause confusion. Consider logging or throwing an error if the input is truly invalid.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
examples/sites/demos/apis/statistic.js
(2 hunks)examples/sites/demos/pc/app/statistic/basic-usage-composition-api.vue
(1 hunks)examples/sites/demos/pc/app/statistic/basic-usage.vue
(1 hunks)examples/sites/demos/pc/app/statistic/statistic-slot.spec.ts
(1 hunks)examples/sites/demos/pc/app/statistic/statistic-style.spec.ts
(0 hunks)packages/renderless/src/statistic/index.ts
(1 hunks)packages/vue/src/statistic/src/index.ts
(1 hunks)
💤 Files with no reviewable changes (1)
- examples/sites/demos/pc/app/statistic/statistic-style.spec.ts
🔇 Additional comments (9)
examples/sites/demos/pc/app/statistic/statistic-slot.spec.ts (2)
6-6
: Looks good: Use of locator is consistent with Playwright best practices.
11-11
: Ensure consistent test coverage for style checks.
While verifying font-weight is valuable, consider adding checks for other CSS properties if they are crucial to the UI consistency (e.g., color, line-height).
packages/vue/src/statistic/src/index.ts (2)
14-14
: Confirm the default precision usage.
Removing the default value shifts responsibility to either the user or upstream logic. Verify the downstream code flow to ensure that no unintended NaN or string parse errors occur when precision
is undefined.
17-17
: Great flexibility for value
type.
Allowing both Number
and String
broadens use cases. Confirm that any formatting or parsing logic in dependent modules handles string values properly (e.g., ensuring correct numeric conversion).
packages/renderless/src/statistic/index.ts (2)
18-18
: Explicitly convert prop to string for robust splitting.
Your conversion to string is appropriate; confirm that multi-lingual or locale-specific numerals don’t introduce unexpected splitting or formatting issues.
20-21
: Potential edge cases for props.precision
.
When props.precision
is not set or is zero, ensure that .padEnd()
does not introduce extraneous zeroes. Some numeric edge cases (e.g., negative values or extremely large numbers) might need additional coverage.
examples/sites/demos/apis/statistic.js (2)
10-10
: Allowing value
to be a string is a good improvement.
Using 'number | string'
broadens the prop's usability. However, consider adding validations or type checks to ensure consistent usage.
23-23
: Empty default value for precision
might be confusing.
Switching from a numeric default to an empty string can lead to unexpected behavior if the downstream logic expects a number. Confirm that the rendering logic handles an empty string correctly, possibly defaulting to 0 if none is provided.
examples/sites/demos/pc/app/statistic/basic-usage-composition-api.vue (1)
11-13
: No explicit precision
may affect display if precision
is no longer numeric by default.
This new column demonstrates the updated default behavior. Ensure that the code that processes the precision
prop can handle an empty string without throwing or rendering anomalies.
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
Improvements
Bug Fixes