-
Notifications
You must be signed in to change notification settings - Fork 273
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] when precision is not set, the current value is displayed by default #2683
Conversation
…lue is displayed by default
WalkthroughThis pull request introduces modifications to the Tiny Statistic component across multiple files. The changes primarily focus on enhancing the flexibility of the component's properties, particularly the Changes
Possibly related PRs
Suggested reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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 (
|
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: 1
🧹 Nitpick comments (5)
examples/sites/demos/pc/app/statistic/basic-usage-composition-api.vue (1)
11-13
: Enhance example documentation and coverageWhile this example demonstrates the default precision behavior, consider:
- Adding comments to explain the expected output
- Including an example with a string value to demonstrate the new type support
<tiny-col :span="8"> + <!-- Demonstrates default precision behavior --> <tiny-statistic :value="num"></tiny-statistic> </tiny-col> + <tiny-col :span="8"> + <!-- Demonstrates string value support --> + <tiny-statistic value="1,234.567"></tiny-statistic> + </tiny-col>examples/sites/demos/pc/app/statistic/basic-usage.vue (1)
11-13
: Maintain parity with composition API examplePlease ensure this example maintains feature parity with the composition API version by including the same improvements:
- Add explanatory comments
- Include string value demonstration
<tiny-col :span="8"> + <!-- Demonstrates default precision behavior --> <tiny-statistic :value="num"></tiny-statistic> </tiny-col> + <tiny-col :span="8"> + <!-- Demonstrates string value support --> + <tiny-statistic value="1,234.567"></tiny-statistic> + </tiny-col>packages/renderless/src/statistic/index.ts (1)
22-25
: Review precision handling logicThe new condition
props.precision >= 0
improves the default value display, but consider these suggestions:
- Add a comment explaining what happens when precision is undefined (current value displayed)
- Consider adding type guards for props.precision to handle non-numeric values
// 当精度为0且大于0,进行精度截取 -if (props.precision >= 0) { +// When precision is not set, display the current value +// When precision >= 0, truncate decimal places +if (typeof props.precision === 'number' && props.precision >= 0) { decimal = decimal?.slice(0, props.precision > 0 ? props.precision : 0) }examples/sites/demos/apis/statistic.js (2)
10-10
: Document migration impact of value type changeThe type change from 'number' to 'number | string' might affect existing implementations. Consider adding a note in the documentation about handling string values.
23-23
: Update precision documentation for clarityThe empty string default value needs more context in the documentation. Consider adding:
- What happens when precision is not set (displays current value)
- Valid range of precision values
- Example usage with different precision values
defaultValue: '', +desc: { + 'zh-CN': '精度值,不设置时显示当前值,设置为0或正整数时进行相应精度截取', + 'en-US': 'Precision value. When not set, displays current value. When set to 0 or positive integer, truncates to specified precision' +},
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
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)packages/renderless/src/statistic/index.ts
(1 hunks)packages/vue/src/statistic/package.json
(1 hunks)packages/vue/src/statistic/src/index.ts
(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/vue/src/statistic/package.json
🔇 Additional comments (2)
packages/vue/src/statistic/src/index.ts (1)
14-14
: 🛠️ Refactor suggestion
Consider maintaining default value for precision
Removing the default value for precision might lead to inconsistent behavior across the application. Consider keeping the default value or documenting the expected behavior when precision is undefined.
- precision: Number,
+ precision: {
+ type: Number,
+ default: undefined
+ },
packages/renderless/src/statistic/index.ts (1)
18-21
: Verify handling of edge cases in value splitting
The current implementation might need additional validation for edge cases:
- When props.value is 0
- When props.value is a non-numeric string
- When props.value is undefined/null
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 (1)
examples/sites/demos/pc/app/statistic/statistic-slot.spec.ts (1)
3-12
: Verify test description matches implementationThe test name "插槽用法" (slot usage) suggests this test is for slot functionality, but the assertions are focused on styling. Consider:
- Splitting these into separate test cases for slots and styling
- Updating the test description to match its actual purpose
-test('插槽用法', async ({ page }) => { +test('Statistic styling - description font weight', async ({ page }) => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
examples/sites/demos/pc/app/statistic/statistic-slot.spec.ts
(1 hunks)examples/sites/demos/pc/app/statistic/statistic-style.spec.ts
(0 hunks)
💤 Files with no reviewable changes (1)
- examples/sites/demos/pc/app/statistic/statistic-style.spec.ts
🔇 Additional comments (1)
examples/sites/demos/pc/app/statistic/statistic-slot.spec.ts (1)
6-11
:
Additional tests needed for precision-related changes
While these test changes verify the styling of the description element, they don't address the main objective of this PR - fixing the display of values when precision is not set. Consider adding specific test cases to verify this functionality.
Let's verify if there are any existing tests for precision handling:
Would you like me to help draft test cases for:
- Default behavior when precision is not set
- Edge cases with different value types
- Validation of formatted output
…lue is displayed by default
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
New Features
value
property in the statistic component to accept both numbers and strings.<tiny-statistic>
components in multiple locations to improve statistical data display.Bug Fixes
Chores
@opentiny/vue-statistic
package from3.18.0
to3.18.1
.