fix(button): modify mobile problem #2660
Conversation
WalkthroughThis pull request introduces modifications to the button and input components in the mobile package. The changes primarily focus on enhancing type flexibility for the Changes
Sequence DiagramsequenceDiagram
participant Button
participant HandleClick
Button->>HandleClick: Trigger click event
HandleClick->>HandleClick: Convert resetTime to number
HandleClick-->>Button: Process reset logic
Possibly related PRs
Suggested labels
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 (
|
|
[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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/mobile/components/button/src/renderless/index.ts (1)
19-19: Consider moving the type conversion to a computed propertyThe
Numberconversion is performed on every click, which is inefficient if theresetTimeprop doesn't change frequently.Consider moving this to a computed property in the component:
const reset = computed(() => Number(props.resetTime))Then use
reset.valuein the click handler.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/mobile/components/button/src/button.ts(2 hunks)packages/mobile/components/button/src/renderless/index.ts(1 hunks)packages/mobile/components/input/src/input.ts(0 hunks)
💤 Files with no reviewable changes (1)
- packages/mobile/components/input/src/input.ts
| @@ -16,15 +16,15 @@ export const handleClick = | |||
| ({ emit, props, state }: Pick<IButtonRenderlessParams, 'emit' | 'props' | 'state'>) => | |||
| (event: MouseEvent): void => { | |||
| const urlHref = xss.filterUrl(props.href) | |||
|
|
|||
| let reset = Number(props.resetTime) | |||
There was a problem hiding this comment.
Add error handling for invalid resetTime values
The current implementation assumes that props.resetTime can always be successfully converted to a valid number. However, since the type now accepts strings, we should handle potential invalid values.
Consider applying this change:
- let reset = Number(props.resetTime)
+ const reset = Number(props.resetTime)
+ if (isNaN(reset)) {
+ console.warn('[Button] Invalid resetTime value:', props.resetTime)
+ return
+ }Also applies to: 22-27
| @@ -29,7 +30,7 @@ export const buttonProps = { | |||
| default: '' | |||
| }, | |||
| resetTime: { | |||
| type: Number, | |||
| type: [Number, String], | |||
There was a problem hiding this comment.
💡 Codebase verification
Type widening in resetTime prop requires attention
The verification reveals inconsistent type definitions for resetTime across the codebase:
- Mobile button component:
type: [Number, String] - Vue button component:
type: Number - Float button component:
type: Number - Floating button component:
type: [String, Number]
This type widening in the mobile button component could cause runtime issues since the renderless logic (packages/mobile/components/button/src/renderless/index.ts) uses Number(props.resetTime) to convert the value, assuming a numeric type.
🔗 Analysis chain
Verify usage of resetTime across the codebase
Since this is a breaking change in TypeScript (widening the type), we should verify all existing usage of this prop.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for resetTime usage in the codebase
rg -t ts -t vue "resetTime" packages/
Length of output: 65
Script:
#!/bin/bash
# Search for resetTime usage in the codebase with ripgrep, including all text files
rg "resetTime" --type-add 'vue:*.vue' -t ts -t vue
Length of output: 1819
Script:
#!/bin/bash
# Let's check the actual type definition of resetTime in these files
ast-grep --pattern 'resetTime: {
$$$
}'
Length of output: 43
Script:
#!/bin/bash
# Let's examine the content of the files where resetTime type is defined
rg -A 5 "resetTime: \{" --type-add 'vue:*.vue' -t ts -t vue
Length of output: 1474
💡 Codebase verification
Add validation and documentation for resetTime prop
The resetTime prop is used in the button component for controlling the disabled state duration, as evidenced by the test case. To ensure proper usage:
- Add JSDoc documentation explaining that
resetTimecontrols the button's re-enable delay in milliseconds - Add validation to ensure numeric strings are convertible to positive numbers
- Consider updating the test to cover string values
/**
+ * @prop {number|string} resetTime - Duration in milliseconds before the button is re-enabled after click
+ * @default 1000
+ */
resetTime: {
type: [Number, String],
default: 1000,
+ validator(val: number | string): boolean {
+ const num = Number(val)
+ return !isNaN(num) && num > 0
+ }
},🔗 Analysis chain
Document accepted string formats and add validation
The resetTime prop now accepts strings, but there's no documentation or validation for acceptable string values. This could lead to runtime errors if invalid strings are provided.
Consider adding:
- JSDoc documentation for the prop
- A custom validator function
resetTime: {
type: [Number, String],
- default: 1000
+ default: 1000,
+ validator(val: number | string): boolean {
+ const num = Number(val)
+ return !isNaN(num) && num >= 0
+ }
},Let's verify if there are any existing tests:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for button component test files
fd -e spec.ts -e spec.js -e test.ts -e test.js button packages/mobile/
Length of output: 72
Script:
#!/bin/bash
# Let's try a broader search for test files and button-related files
fd "button.*\.(spec|test)\.(ts|js|tsx|jsx)" .
# Also search for any button test files in common test directories
fd "button.*\.(spec|test)\.(ts|js|tsx|jsx)" -p "tests?/" .
# Let's also check the implementation to see how resetTime is used
ast-grep --pattern 'resetTime: {
$$$
}'
# And check for any existing JSDoc documentation
rg "@prop.*resetTime" -A 3
Length of output: 3742
Script:
#!/bin/bash
# Let's check the content of the mobile button reset-time test
cat "./examples/sites/demos/mobile/app/button/reset-time.spec.ts"
# Let's also check how resetTime is used in the implementation
rg "resetTime" -A 3 -B 3 packages/mobile/components/button/src/button.ts
# Check if there are any existing validator functions in the button component
ast-grep --pattern 'validator($$$) {
$$$
}'
Length of output: 713
| @@ -29,7 +30,7 @@ export const buttonProps = { | |||
| default: '' | |||
| }, | |||
| resetTime: { | |||
| type: Number, | |||
| type: [Number, String], | |||
There was a problem hiding this comment.
这里为什么需要拓展一下类型，resetTime传Number更符合直觉
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
resetTimeproperty to accept bothNumberandStringtypes.titleproperty ininputPropsto include a default value and improved type safety for thewidthproperty.Bug Fixes
resetTimeproperty to consistently treat it as a number, preventing potential type-related issues.