feat(numeric): [numeric] add step strategy of props #2584
Conversation
WalkthroughThe changes in this pull request primarily focus on enhancing the Changes
Possibly related PRs
Suggested reviewers
Poem
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: 7
🧹 Outside diff range and nitpick comments (9)
examples/sites/demos/pc/app/numeric/about-step.spec.ts (4)
6-7: Use more specific selectors for locatorsThe locators for
stepandincreaseuse CSS selectors that may be brittle and prone to breaking if the DOM structure changes.Consider using roles or data attributes for more resilient selectors. For example:
-const step = page.locator('#about-step') -const increase = page.locator('div:nth-child(2) > .tiny-numeric-base > .tiny-numeric__increase') +const step = page.locator('[data-test-id="about-step"]') +const increase = page.locator('[data-test-id="increase-button"]')
9-11: Add assertions to verify the initial state before actionsBefore performing actions like clicks and fills, it's good practice to assert the initial state to ensure the test is reliable.
Consider adding assertions like:
expect(await input.first().inputValue()).toEqual('0')
20-22: Consistent use of locatorsThe locator for
increaseusesincrease.nth(1), while forstep, it usesstep.getByRole('spinbutton').nth(1). This inconsistency can lead to confusion.Consider using consistent locator strategies for better readability.
31-33: Potential flaky test due to hardcoded selectorsThe selector
'div:nth-child(2) > .tiny-numeric > .tiny-numeric-base > .tiny-numeric__increase'can be fragile.Prefer using more robust selectors, such as data attributes or roles.
packages/renderless/src/numeric/index.ts (1)
456-458: Typo in function namegetNumPecisionThe function name
getNumPecisionappears to have a typo.Consider renaming it to
getNumPrecisionfor clarity.examples/sites/demos/pc/app/numeric/about-step-composition-api.vue (2)
11-11: Inconsistent use ofsteppropertyIn the third
tiny-numeric,:step="step.value"is used withstep-strictlyprop, butstepStrictlyis also set instepRestrictly.Consider using a consistent approach by either passing the entire
stepobject or adjusting the configuration.
33-41: Scoped styles affecting global elementsThe styles defined may unintentionally affect other components.
Ensure the styles are properly scoped to prevent side effects.
examples/sites/demos/pc/app/numeric/about-step.vue (1)
11-11: Potential confusion withstepusageUsing
:step="step.value"along withstep-strictlymight cause confusion.Consider passing the entire
stepobject if it includes mode configurations, or clarify the intent.examples/sites/demos/apis/numeric.js (1)
521-528: Consider adding JSDoc comments to the IStepStrategy interface.While the interface is well-structured, adding JSDoc comments would improve the API documentation:
interface IStepStrategy { + /** The step value to be used */ value: string | number, + /** The step mode: 'strictly' enforces step multiples, 'restore' adjusts to nearest step */ mode: 'strictly' | 'restore' }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
examples/sites/demos/apis/numeric.js(3 hunks)examples/sites/demos/pc/app/numeric/about-step-composition-api.vue(1 hunks)examples/sites/demos/pc/app/numeric/about-step.spec.ts(1 hunks)examples/sites/demos/pc/app/numeric/about-step.vue(2 hunks)examples/sites/demos/pc/app/numeric/webdoc/numeric.js(1 hunks)packages/renderless/src/numeric/index.ts(6 hunks)packages/vue/src/numeric/src/index.ts(1 hunks)
🔇 Additional comments (8)
examples/sites/demos/pc/app/numeric/about-step.spec.ts (1)
17-18: Ensure correct value after increment
The test expects the value to be 5, but without prior context, it's unclear if 5 is the correct expected value after the increment.
Please verify that the step configuration and initial value result in 5 after one increment.
examples/sites/demos/pc/app/numeric/about-step-composition-api.vue (2)
5-7: Verify binding of v-model properties
The tiny-numeric components are using v-model="value" and v-model="restrictValue", but the variables need to be defined.
Ensure that value and restrictValue are declared in the script section.
21-30: Unused variables in script
Variables restrictValue and valueStep are declared but may not be properly initialized or used.
Confirm that these variables are correctly initialized and bound to the template.
examples/sites/demos/pc/app/numeric/about-step.vue (2)
5-7: Ensure all v-model bindings have corresponding data properties
The tiny-numeric components use v-model="value", v-model="restrictValue", and v-model="valueStep". Verify that these are defined.
Check the data section to confirm these properties are declared.
26-35: Validate the structure of step and stepRestrictly objects
Ensure that the step and stepRestrictly objects have the correct structure and that their properties are being used appropriately.
Confirm that these objects align with the expected API of the tiny-numeric component.
examples/sites/demos/pc/app/numeric/webdoc/numeric.js (1)
38-41: LGTM! Documentation updates are clear and comprehensive.
The description accurately reflects the new step functionality, including:
- New
modeoptions (restoreandstrictly) - Introduction of
step-strictlyattribute
examples/sites/demos/apis/numeric.js (2)
292-297: LGTM! Step property enhancement is well-defined.
The type expansion and version metadata addition are appropriate for tracking the new functionality.
292-297: Verify integration with existing numeric implementations.
The new step strategy feature introduces significant changes to the numeric component's behavior.
Also applies to: 521-528
✅ Verification successful
Based on the comprehensive search results, I can now provide a final response:
The step strategy feature is well-integrated and properly documented.
The verification shows:
- The
IStepStrategyinterface is properly defined and documented in the API - Multiple demo implementations showcase the step functionality working correctly:
- Basic step usage
- Step-strictly mode
- Step with different modes (restore/strictly)
- Tests verify the step functionality for both button clicks and keyboard controls
- The feature is consistently implemented across PC, mobile, and mobile-first versions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify existing numeric implementations won't break with new step strategy
# Search for current numeric component usage patterns
echo "Checking current numeric component usage patterns..."
rg -l "(?s)<tiny-numeric[^>]*step=" --type vue
# Search for potential conflicts with existing step implementations
echo "Checking for potential conflicts..."
rg -l "(?s)<tiny-numeric[^>]*step=.*?mode=" --type vue
Length of output: 357
Script:
#!/bin/bash
# Let's try a different approach to find numeric component usage patterns
# First, let's find all potential UI files
echo "Finding all potential UI files..."
fd -e js -e jsx -e ts -e tsx -e vue
# Then search for numeric component usage in these files
echo "Checking numeric component usage patterns..."
rg -l "tiny-numeric"
# Look for step-related implementations
echo "Checking step-related implementations..."
rg -C 3 "step.*?="
# Check the numeric component's implementation
echo "Examining numeric component implementation..."
fd -t f numeric
Length of output: 74079
Script:
#!/bin/bash
# Let's search for numeric component implementations and step-related code
echo "Searching for numeric component implementations..."
rg -C 5 "IStepStrategy"
echo "Checking numeric component usage..."
rg -C 5 "tiny-numeric.*?step="
echo "Looking for step-related code..."
fd -t f numeric | xargs rg -C 5 "step"
Length of output: 26285
| let newVal = api.internalDecrease({ | ||
| val: value, | ||
| step: props.step.value ? Number(props.step.value) : props.step | ||
| }) |
There was a problem hiding this comment.
Consistent handling of step in decrease function
Same issue as in increase function; ensure props.step is handled correctly.
Apply the same fix as above for the decrease function.
| ({ val, step }: { val: number; step: number | string }): string => { | ||
| const decimal = api.getDecimal(val) | ||
|
|
||
| if (decimal.isNaN() && val !== undefined) { | ||
| return state.currentValue | ||
| } | ||
|
|
||
| return decimal.add(step).toString() | ||
| const addValue = decimal.add(step).toString() | ||
| return addValue |
There was a problem hiding this comment.
Handle string inputs for step parameter
The internalIncrease function now accepts step as number | string, but decimal.add(step) may not handle strings properly.
Ensure step is converted to a number before using it in calculations. Apply this fix:
const addValue = decimal.add(Number(step)).toString()📝 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.
| ({ val, step }: { val: number; step: number | string }): string => { | |
| const decimal = api.getDecimal(val) | |
| if (decimal.isNaN() && val !== undefined) { | |
| return state.currentValue | |
| } | |
| return decimal.add(step).toString() | |
| const addValue = decimal.add(step).toString() | |
| return addValue | |
| ({ val, step }: { val: number; step: number | string }): string => { | |
| const decimal = api.getDecimal(val) | |
| if (decimal.isNaN() && val !== undefined) { | |
| return state.currentValue | |
| } | |
| const addValue = decimal.add(Number(step)).toString() | |
| return addValue |
| ({ val, step }: { val: number; step: number | string }): string | number => { | ||
| const decimal = api.getDecimal(val) | ||
|
|
||
| if (decimal.isNaN() && val !== undefined) { | ||
| return state.currentValue | ||
| } | ||
|
|
||
| return decimal.add(`-${step}`).toString() | ||
| const decreaseValue = decimal.add(`-${step}`).toString() | ||
| return decreaseValue |
There was a problem hiding this comment.
Handle string inputs for step parameter in internalDecrease
Similar to internalIncrease, internalDecrease should ensure step is a number.
Convert step to a number before using it:
const decreaseValue = decimal.add(-Number(step)).toString()📝 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.
| ({ val, step }: { val: number; step: number | string }): string | number => { | |
| const decimal = api.getDecimal(val) | |
| if (decimal.isNaN() && val !== undefined) { | |
| return state.currentValue | |
| } | |
| return decimal.add(`-${step}`).toString() | |
| const decreaseValue = decimal.add(`-${step}`).toString() | |
| return decreaseValue | |
| ({ val, step }: { val: number; step: number | string }): string | number => { | |
| const decimal = api.getDecimal(val) | |
| if (decimal.isNaN() && val !== undefined) { | |
| return state.currentValue | |
| } | |
| const decreaseValue = decimal.add(-Number(step)).toString() | |
| return decreaseValue |
| if (props.stepStrictly || props.step?.mode === 'strictly') { | ||
| const previousValue = Number((props.mouseWheel ? state.displayValue : props.modelValue) || 0) | ||
| if (Math.abs(previousValue - value) % Number(props.step) === 0) return api.setCurrentValue(value) | ||
| const step = Number(props.step) | ||
| if ( | ||
| Math.abs(previousValue - value) % Number(props.step) === 0 || | ||
| Math.abs(previousValue - value) % Number(props.step.value) === 0 | ||
| ) | ||
| return api.setCurrentValue(value) | ||
| const step = Number(props.step) || Number(props.step.value) |
There was a problem hiding this comment.
Incorrect modulus operation with props.step
In the condition, Math.abs(previousValue - value) % Number(props.step) assumes props.step is always a number.
Ensure props.step is converted properly, and handle cases where props.step is an object:
const stepValue = typeof props.step === 'object' ? Number(props.step.value) : Number(props.step)
if (Math.abs(previousValue - value) % stepValue === 0) {
return api.setCurrentValue(value)
}📝 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.
| if (props.stepStrictly || props.step?.mode === 'strictly') { | |
| const previousValue = Number((props.mouseWheel ? state.displayValue : props.modelValue) || 0) | |
| if (Math.abs(previousValue - value) % Number(props.step) === 0) return api.setCurrentValue(value) | |
| const step = Number(props.step) | |
| if ( | |
| Math.abs(previousValue - value) % Number(props.step) === 0 || | |
| Math.abs(previousValue - value) % Number(props.step.value) === 0 | |
| ) | |
| return api.setCurrentValue(value) | |
| const step = Number(props.step) || Number(props.step.value) | |
| if (props.stepStrictly || props.step?.mode === 'strictly') { | |
| const previousValue = Number((props.mouseWheel ? state.displayValue : props.modelValue) || 0) | |
| const stepValue = typeof props.step === 'object' ? Number(props.step.value) : Number(props.step) | |
| if (Math.abs(previousValue - value) % stepValue === 0) { | |
| return api.setCurrentValue(value) | |
| } | |
| const step = Number(props.step) || Number(props.step.value) |
| type: [Number, String, Object], | ||
| default: 1 | ||
| }, |
There was a problem hiding this comment.
Update type definitions for step prop
The step prop now includes Object in its type without a corresponding interface or type definition.
Define an interface for the step object to ensure type safety and maintainability.
interface StepStrategy {
value: number | string
mode: 'restore' | 'strictly'
}Then, update the prop type:
step: {
type: [Number, String, Object] as PropType<number | string | StepStrategy>,
default: 1
}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/renderless/src/numeric/index.ts(6 hunks)
🔇 Additional comments (4)
packages/renderless/src/numeric/index.ts (4)
309-316:
Improve restore mode validation in setCurrentValue
The current implementation has issues with decimal steps and lacks proper validation.
Apply these improvements:
if (props.step instanceof Object && props.step?.mode === 'restore' && props.step?.value) {
const stepValue = Number(props.step.value)
- if (stepValue > 1 && newVal % stepValue !== 0) {
+ if (!isNaN(stepValue) && stepValue > 0 && newVal % stepValue !== 0) {
state.currentValue = oldVal
state.userInput = oldVal
}
}Likely invalid or redundant comment.
87-93: 🛠️ Refactor suggestion
Improve type safety and precision handling in internalIncrease
The function needs better validation and precision handling for string inputs.
Apply this improvement:
({ val, step }: { val: number; step: number | string }): string => {
const decimal = api.getDecimal(val)
if (decimal.isNaN() && val !== undefined) {
return state.currentValue
}
- const addValue = decimal.add(step).toString()
+ const stepValue = typeof step === 'string' ? parseFloat(step) : step
+ if (isNaN(stepValue)) {
+ return state.currentValue
+ }
+ const addValue = decimal.add(stepValue).toString()
return addValue
}Likely invalid or redundant comment.
98-106: 🛠️ Refactor suggestion
Improve string handling in internalDecrease
Similar to internalIncrease, the function needs better string handling.
Apply this improvement:
({ val, step }: { val: number; step: number | string }): string | number => {
const decimal = api.getDecimal(val)
if (decimal.isNaN() && val !== undefined) {
return state.currentValue
}
- const decreaseValue = decimal.add(`-${step}`).toString()
+ const stepValue = typeof step === 'string' ? parseFloat(step) : step
+ if (isNaN(stepValue)) {
+ return state.currentValue
+ }
+ const decreaseValue = decimal.add(-stepValue).toString()
return decreaseValue
}Likely invalid or redundant comment.
Line range hint 1-24: Verify usage of new step object structure
The changes introduce a new object structure for the step property. We should verify all usages across the codebase.
| let newVal = api.internalIncrease({ | ||
| val: value, | ||
| step: props.step.value ? Number(props.step.value) : props.step | ||
| }) |
There was a problem hiding this comment.
Add type safety checks for step object access
Both increase and decrease functions need safer type checking when accessing step.value.
Apply this improvement to both functions:
let newVal = api.internalIncrease({
val: value,
- step: props.step.value ? Number(props.step.value) : props.step
+ step: typeof props.step === 'object' && props.step?.value !== undefined
+ ? Number(props.step.value)
+ : Number(props.step)
})Also applies to: 156-159
There was a problem hiding this comment.
props.step.value取值是应该要有容错措施，先判断props.step是不是一个对象
There was a problem hiding this comment.
@zzcr，感谢您的理解和确认。是的，在访问props.step.value之前，应该先判断props.step是否是一个对象，以提高代码的健壮性。
(^-^)/
✏️ Learnings added
Learnt from: zzcr
PR: opentiny/tiny-vue#2584
File: packages/renderless/src/numeric/index.ts:124-127
Timestamp: 2024-12-02T08:56:05.048Z
Learning: 在`packages/renderless/src/numeric/index.ts`文件中，在访问`props.step.value`之前，应先判断`props.step`是否为对象，以确保容错处理。
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.
| @@ -437,7 +453,7 @@ | |||
| export const getNumPecision = | |||
| ({ api, props }: Pick<INumericRenderlessParams, 'api' | 'props'>) => | |||
| (): number => { | |||
| const stepPrecision = api.getPrecision(props.step) | |||
| const stepPrecision = api.getPrecision(props.step.value || props.step) | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add type safety to precision calculation
The function needs proper type checking before accessing step.value.
Apply this improvement:
-const stepPrecision = api.getPrecision(props.step.value || props.step)
+const stepValue = typeof props.step === 'object' && props.step?.value !== undefined
+ ? props.step.value
+ : props.step
+const stepPrecision = api.getPrecision(stepValue)📝 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.
| const stepPrecision = api.getPrecision(props.step.value || props.step) | |
| const stepValue = typeof props.step === 'object' && props.step?.value !== undefined | |
| ? props.step.value | |
| : props.step | |
| const stepPrecision = api.getPrecision(stepValue) |
| if (props.stepStrictly || props.step.mode === 'strictly') { | ||
| const previousValue = Number((props.mouseWheel ? state.displayValue : props.modelValue) || 0) | ||
| if (Math.abs(previousValue - value) % Number(props.step) === 0) return api.setCurrentValue(value) | ||
| const step = Number(props.step) | ||
| if ( | ||
| Math.abs(previousValue - value) % Number(props.step) === 0 || | ||
| Math.abs(previousValue - value) % Number(props.step.value) === 0 | ||
| ) | ||
| return api.setCurrentValue(value) | ||
| const step = Number(props.step) || Number(props.step.value) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify step validation logic in handleInputChange
The current implementation has duplicate calculations and potential undefined access.
Apply these improvements:
-if (props.stepStrictly || props.step.mode === 'strictly') {
+if (props.stepStrictly || (typeof props.step === 'object' && props.step?.mode === 'strictly')) {
const previousValue = Number((props.mouseWheel ? state.displayValue : props.modelValue) || 0)
- if (
- Math.abs(previousValue - value) % Number(props.step) === 0 ||
- Math.abs(previousValue - value) % Number(props.step.value) === 0
- )
+ const stepValue = typeof props.step === 'object' ? Number(props.step.value) : Number(props.step)
+ if (!isNaN(stepValue) && Math.abs(previousValue - value) % stepValue === 0)
return api.setCurrentValue(value)
- const step = Number(props.step) || Number(props.step.value)
const difference = value - previousValue
const sign = difference >= 0 ? 1 : -1
- return api.setCurrentValue(sign * Math.round(Math.abs(difference) / step) * step + previousValue)
+ return api.setCurrentValue(sign * Math.round(Math.abs(difference) / stepValue) * stepValue + previousValue)📝 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.
| if (props.stepStrictly || props.step.mode === 'strictly') { | |
| const previousValue = Number((props.mouseWheel ? state.displayValue : props.modelValue) || 0) | |
| if (Math.abs(previousValue - value) % Number(props.step) === 0) return api.setCurrentValue(value) | |
| const step = Number(props.step) | |
| if ( | |
| Math.abs(previousValue - value) % Number(props.step) === 0 || | |
| Math.abs(previousValue - value) % Number(props.step.value) === 0 | |
| ) | |
| return api.setCurrentValue(value) | |
| const step = Number(props.step) || Number(props.step.value) | |
| if (props.stepStrictly || (typeof props.step === 'object' && props.step?.mode === 'strictly')) { | |
| const previousValue = Number((props.mouseWheel ? state.displayValue : props.modelValue) || 0) | |
| const stepValue = typeof props.step === 'object' ? Number(props.step.value) : Number(props.step) | |
| if (!isNaN(stepValue) && Math.abs(previousValue - value) % stepValue === 0) | |
| return api.setCurrentValue(value) | |
| const difference = value - previousValue | |
| const sign = difference >= 0 ? 1 : -1 | |
| return api.setCurrentValue(sign * Math.round(Math.abs(difference) / stepValue) * stepValue + previousValue) |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
examples/sites/demos/apis/numeric.js (1)
322-322: Consider fixing similar type casing inconsistencies.The type has been correctly updated from 'Boolean' to 'boolean'. However, there are other properties in the file still using 'Boolean' (e.g., 'display-only', 'hide-unit', 'hold-zero', etc.).
Consider updating all Boolean types to lowercase 'boolean' for consistency:
- 'display-only'
- 'hide-unit'
- 'hold-zero'
- 'model-truncation'
- 'string-mode'
- 'tabindex'
- 'unit-center'
- 'validate-event'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
examples/sites/demos/apis/numeric.js(3 hunks)
🔇 Additional comments (3)
examples/sites/demos/apis/numeric.js (3)
316-316: LGTM! Demo reference updated for consistency.
The pcDemo value has been updated to 'about-step' to maintain consistency with the step property's demo reference.
Let's verify the demo file exists:
✅ Verification successful
Demo reference update verified successfully
The about-step demo files exist at the expected location in examples/sites/demos/pc/app/numeric/, confirming that the pcDemo value update to 'about-step' is correct and properly aligned with the available demo files.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence of the about-step demo file
fd "about-step" examples/sites/demos/pc/app/numeric/
Length of output: 225
518-527: LGTM! Well-structured interface definitions.
The new interfaces are well-defined:
INumericFormat: Comprehensive formatting options with clear descriptionsIStepStrategy: Clear structure for the new step configuration with proper type constraints
Let's verify the usage of these interfaces in the codebase:
✅ Verification successful
Both interfaces are properly defined and used within the codebase
The interfaces are well integrated:
INumericFormatis used as a type definition and referenced in documentationIStepStrategyis correctly used as a union type (number | string | IStepStrategy) for configuration options
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for usage of the new interfaces
echo "Checking INumericFormat usage:"
rg "INumericFormat" --type ts
echo "Checking IStepStrategy usage:"
rg "IStepStrategy" --type ts
Length of output: 198
Script:
#!/bin/bash
# Let's try searching without type restriction since the file is .js
echo "Checking INumericFormat usage:"
rg "INumericFormat" -A 2
echo -e "\nChecking IStepStrategy usage:"
rg "IStepStrategy" -A 2
Length of output: 1869
292-297: LGTM! Enhanced step property with new type options.
The step property has been expanded to support more flexible configurations through the new IStepStrategy interface. The stability version is properly documented.
Let's verify the usage of the new step property type in demos:
✅ Verification successful
After analyzing the code output, I can see that the new IStepStrategy interface and step property changes are being used correctly in the demos. Specifically, in the "about-step" demo files, there are examples using the step property with both restore and strictly modes:
step: {
value: 5,
mode: 'restore'
},
stepRestrictly: {
value: 5,
mode: 'strictly'
}The step property enhancement is properly implemented and demonstrated.
- The new
IStepStrategyinterface is correctly referenced in the API documentation - The step property is demonstrated with both object notation and simple number values
- The demos show proper usage of both
restoreandstrictlymodes - The feature is correctly marked as stable in version 3.20.0
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for examples using the new step strategy
rg -l "step.*=.*{.*value.*mode" examples/sites/demos/pc/app/numeric/
Length of output: 186
Script:
#!/bin/bash
# Search for step property usage in numeric demos with better regex
rg -l 'step\s*[:=]\s*{' examples/sites/demos/pc/app/numeric/
# Also check for any files that might contain the new IStepStrategy interface
rg -l 'IStepStrategy' .
Length of output: 219
Script:
#!/bin/bash
# Search for step property usage in numeric demos with fixed regex
rg 'step[[:space:]]*[:=][[:space:]]*{' examples/sites/demos/pc/app/numeric/
# Check the content of numeric demos directory for any step-related examples
fd . examples/sites/demos/pc/app/numeric/ -t f -x cat {}
Length of output: 35243
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
packages/renderless/src/numeric/index.ts (2)
124-127: Extract common step value logicThe step value extraction logic is duplicated. Consider extracting it into a helper function.
Create a helper function:
+const getStepValue = (step: any): number | string => { + return typeof step === 'object' && step?.value !== undefined ? step.value : step +} export const increase = ({ api, props, state }: Pick<INumericRenderlessParams, 'api' | 'props' | 'state'>) => (): void => { // ... let newVal = api.internalIncrease({ val: value, - step: typeof props.step === 'object' && props.step?.value !== undefined ? props.step.value : props.step + step: getStepValue(props.step) }) // ... } export const decrease = ({ api, props, state }: Pick<INumericRenderlessParams, 'api' | 'props' | 'state'>) => (): void => { // ... let newVal = api.internalDecrease({ val: value, - step: typeof props.step === 'object' && props.step?.value !== undefined ? props.step.value : props.step + step: getStepValue(props.step) }) // ... }Also applies to: 156-159
456-457: Use extracted helper functionThis code duplicates the step value extraction logic. Use the proposed
getStepValuehelper function.Apply this change:
-const stepValue = typeof props.step === 'object' && props.step?.value !== undefined ? props.step.value : props.step +const stepValue = getStepValue(props.step) const stepPrecision = api.getPrecision(stepValue)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/renderless/src/numeric/index.ts(6 hunks)
🧰 Additional context used
📓 Learnings (1)
packages/renderless/src/numeric/index.ts (1)
Learnt from: zzcr
PR: opentiny/tiny-vue#2584
File: packages/renderless/src/numeric/index.ts:124-127
Timestamp: 2024-12-02T08:56:05.176Z
Learning: 在`packages/renderless/src/numeric/index.ts`文件中，在访问`props.step.value`之前，应先判断`props.step`是否为对象，以确保容错处理。
🔇 Additional comments (2)
packages/renderless/src/numeric/index.ts (2)
309-316:
Fix restore mode condition for all step values
The current condition stepValue > 1 fails to handle step values less than or equal to 1.
Apply this fix:
if (props.step instanceof Object && props.step?.mode === 'restore' && props.step?.value) {
const stepValue = Number(props.step.value)
- if (stepValue > 1 && newVal % stepValue !== 0) {
+ if (stepValue && newVal % stepValue !== 0) {
state.currentValue = oldVal
state.userInput = oldVal
}
}Likely invalid or redundant comment.
366-373: 🛠️ Refactor suggestion
Simplify step validation logic
The current implementation has duplicate calculations and potential undefined access.
Apply these improvements:
-if (props.stepStrictly || (typeof props.step === 'object' && props.step?.mode === 'strictly')) {
+if (props.stepStrictly || (typeof props.step === 'object' && props.step?.mode === 'strictly')) {
const previousValue = Number((props.mouseWheel ? state.displayValue : props.modelValue) || 0)
- if (
- Math.abs(previousValue - value) % Number(props.step) === 0 ||
- Math.abs(previousValue - value) % Number(props.step.value) === 0
- )
+ const stepValue = typeof props.step === 'object' ? Number(props.step.value) : Number(props.step)
+ if (!isNaN(stepValue) && Math.abs(previousValue - value) % stepValue === 0)
return api.setCurrentValue(value)
- const step = Number(props.step) || Number(props.step.value)
const difference = value - previousValue
const sign = difference >= 0 ? 1 : -1
- return api.setCurrentValue(sign * Math.round(Math.abs(difference) / step) * step + previousValue)
+ return api.setCurrentValue(sign * Math.round(Math.abs(difference) / stepValue) * stepValue + previousValue)Likely invalid or redundant comment.
| ({ val, step }: { val: number | string; step: number | string }): string => { | ||
| const decimal = api.getDecimal(val) | ||
|
|
||
| if (decimal.isNaN() && val !== undefined) { | ||
| return state.currentValue | ||
| } | ||
|
|
||
| return decimal.add(step).toString() | ||
| const addValue = decimal.add(step).toString() | ||
| return addValue |
There was a problem hiding this comment.
Add type conversion for arithmetic operations
The functions now accept string inputs but don't ensure numeric conversion before arithmetic operations. This could lead to unexpected results.
Apply these changes:
export const internalIncrease =
({ api, state }: Pick<INumericRenderlessParams, 'api' | 'state'>) =>
({ val, step }: { val: number | string; step: number | string }): string => {
const decimal = api.getDecimal(val)
if (decimal.isNaN() && val !== undefined) {
return state.currentValue
}
- const addValue = decimal.add(step).toString()
+ const addValue = decimal.add(Number(step)).toString()
return addValue
}
export const internalDecrease =
({ api, state }: Pick<INumericRenderlessParams, 'api' | 'state'>) =>
({ val, step }: { val: number | string; step: number | string }): string | number => {
const decimal = api.getDecimal(val)
if (decimal.isNaN() && val !== undefined) {
return state.currentValue
}
- const decreaseValue = decimal.add(`-${step}`).toString()
+ const decreaseValue = decimal.add(-Number(step)).toString()
return decreaseValue
}Also applies to: 98-106
| let newVal = api.internalDecrease({ val: value, step: props.step }) | ||
| let newVal = api.internalDecrease({ | ||
| val: value, | ||
| step: typeof props.step === 'object' && props.step?.value !== undefined ? props.step.value : props.step |
There was a problem hiding this comment.
机器人给的写法太长了，建议用这个：props.step?.value ?? props.step
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
stepproperty to accept more complex configurations including numbers, strings, and objects.step-strictlyfunctionality to restrict inputs to multiples of the step value.Improvements
tiny-numericcomponent usage.about-stepdemo to clarify usage of modes.Bug Fixes