Skip to content
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

[WIP]:feat(numeric): [numeric] add props of numeric #2546

Closed
wants to merge 1 commit into from

Conversation

James-9696
Copy link
Collaborator

@James-9696 James-9696 commented Nov 22, 2024

PR

PR Checklist

Please check if your PR fulfills the following requirements:

  • The commit message follows our Commit Message Guidelines
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • Other... Please describe:

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

Summary by CodeRabbit

Release Notes

  • New Features

    • Introduced step-restore property for the numeric component, allowing restoration of previous values when inputs are not multiples of the step size.
    • Updated step property to enhance demo scenarios with a new value.
    • Added examples demonstrating the usage of step-restore and step-strictly attributes in the numeric component.
  • Bug Fixes

    • Improved handling of numeric input values to ensure consistency with defined step increments.
  • Documentation

    • Enhanced descriptions for the step-restore and step-strictly attributes in both English and Chinese versions for better user guidance.

Copy link

coderabbitai bot commented Nov 22, 2024

Walkthrough

The changes in this pull request enhance the numeric component's API by introducing a new property step-restore and updating existing properties for consistency. The about-step-composition-api.vue and about-step.vue files have been modified to demonstrate the new property and its usage. Additionally, the test cases for the numeric functionality have been updated to reflect these changes. Overall, the updates aim to improve the clarity and functionality of the numeric component and its associated examples.

Changes

File Path Change Summary
examples/sites/demos/apis/numeric.js Added step-restore property (boolean, default false); updated strict-input and step properties.
examples/sites/demos/pc/app/numeric/about-step-composition-api.vue Modified template to include step-restore and step-strictly; updated step value to 5; added valueStep variable.
examples/sites/demos/pc/app/numeric/about-step.vue Modified template to include step-restore and step-strictly; updated step value to 5; added valueStep variable.
examples/sites/demos/pc/app/numeric/about-step.spec.ts Updated test cases to reflect new interactions and assertions for step-restore and step-strictly.
examples/sites/demos/pc/app/numeric/webdoc/numeric.js Enhanced descriptions for step-restore and step-strictly in demo object for clarity.
packages/renderless/src/numeric/index.ts Modified setCurrentValue function to handle stepRestore logic.
packages/vue/src/numeric/src/index.ts Added stepRestore property to numericProps.
packages/vue/src/numeric/src/pc.vue Added stepRestore prop to component's props declaration; no other significant changes.

Possibly related PRs

Suggested labels

enhancement

Suggested reviewers

  • zzcr

🐰 In the land of numbers, where values play,
A new step-restore joins the fray.
With strict inputs and demos anew,
Our tiny-numeric shines bright, it's true!
So hop along, let the values flow,
With updates galore, watch the magic grow! 🌟


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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@github-actions github-actions bot added the enhancement New feature or request (功能增强) label Nov 22, 2024
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 6

🧹 Outside diff range and nitpick comments (14)
examples/sites/demos/pc/app/numeric/about-step-composition-api.vue (3)

5-5: Add ARIA labels and documentation comments

The numeric inputs lack accessibility attributes and explanatory comments about their behavior.

-      <tiny-numeric v-model="value" :step="step" step-restore></tiny-numeric>
+      <!-- When input is not a multiple of step, restores to previous valid value -->
+      <tiny-numeric
+        v-model="value"
+        :step="step"
+        step-restore
+        aria-label="Step restore example"
+      ></tiny-numeric>

-      <tiny-numeric v-model="valueStep" :step="step" step-strictly></tiny-numeric>
+      <!-- Only allows input of values that are multiples of step -->
+      <tiny-numeric
+        v-model="valueStep"
+        :step="step"
+        step-strictly
+        aria-label="Step strictly example"
+      ></tiny-numeric>

Also applies to: 9-9


19-20: Add type annotations for better type safety

Consider adding explicit type annotations to the reactive references.

-const step = ref(5)
-const valueStep = ref(0)
+const step = ref<number>(5)
+const valueStep = ref<number>(0)

24-30: Consider using design system utility classes

Instead of defining custom margin and layout classes, consider using standardized utility classes from your design system.

This would improve consistency across examples and reduce CSS duplication. Consider creating a shared set of utility classes if they don't exist yet.

examples/sites/demos/pc/app/numeric/about-step.vue (3)

5-5: Add ARIA labels and documentation comments

The numeric inputs lack accessibility attributes and the difference between step-restore and step-strictly behaviors should be documented.

-      <tiny-numeric v-model="value" :step="step" step-restore></tiny-numeric>
+      <!-- When step-restore is enabled, the value will be restored to the previous valid step multiple if an invalid value is entered -->
+      <tiny-numeric
+        v-model="value"
+        :step="step"
+        step-restore
+        :aria-label="t('numeric.stepRestore.label')"
+      ></tiny-numeric>
-      <tiny-numeric v-model="valueStep" :step="step" step-strictly></tiny-numeric>
+      <!-- When step-strictly is enabled, only multiples of step value are allowed -->
+      <tiny-numeric
+        v-model="valueStep"
+        :step="step"
+        step-strictly
+        :aria-label="t('numeric.stepStrictly.label')"
+      ></tiny-numeric>

Also applies to: 9-9


24-25: Add TypeScript type definitions and property validation

The data properties lack type definitions and validation.

    return {
-      value: 0,
-      valueStep: 0,
-      step: 5
+      value: 0 as number,
+      valueStep: 0 as number,
+      step: 5 as number
    }

Also, please add the pending unit tests for these properties as mentioned in the PR objectives.

Would you like me to help generate the test cases for these properties?


32-38: Consider using consistent spacing units and responsive layout

The styles could be improved for better maintainability and responsiveness.

 .m-layout {
   display: flex;
   flex-direction: column;
+  gap: 1rem;
 }
 .m-b10 {
-  margin-bottom: 10px;
+  margin-bottom: 1rem;
 }
examples/sites/demos/pc/app/numeric/about-step.spec.ts (2)

Line range hint 1-25: Restructure tests for better organization and coverage

The test file would benefit from better organization and additional test coverage:

  1. Separate test cases for each feature
  2. Add negative test cases
  3. Test error handling
  4. Validate UI states

Consider restructuring like this:

import { test, expect } from '@playwright/test'

describe('Numeric Step Features', () => {
  beforeEach(async ({ page }) => {
    page.on('pageerror', (exception) => expect(exception).toBeNull())
    await page.goto('numeric#about-step')
  })

  describe('step-restore', () => {
    test('should restore to valid step multiple', async ({ page }) => {
      // Current test implementation
    })

    test('should handle negative values', async ({ page }) => {
      // Test negative numbers
    })

    test('should handle disabled state', async ({ page }) => {
      // Test when component is disabled
    })
  })

  describe('step-strictly', () => {
    test('should round to nearest step multiple', async ({ page }) => {
      // Current test implementation
    })

    test('should handle invalid input', async ({ page }) => {
      // Test non-numeric input
    })

    test('should respect min/max bounds', async ({ page }) => {
      // Test boundary conditions
    })
  })
})

Line range hint 1-1: Add JSDoc comments and update documentation

The PR objectives indicate that documentation updates are pending. Please add JSDoc comments to describe the test scenarios and update the component documentation to explain the new step-restore and step-strictly features.

Would you like help generating the documentation for these new features?

packages/vue/src/numeric/src/index.ts (1)

100-103: Add JSDoc documentation for the new stepRestore property

The new property lacks documentation explaining its purpose and usage. Please add JSDoc comments to describe:

  • The property's purpose
  • When to use it
  • Example usage

Example documentation format:

+/**
+ * When true, automatically adjusts the input value to the nearest step multiple when the input loses focus.
+ * @default false
+ */
 stepRestore: {
   type: Boolean,
   default: false
 },
packages/vue/src/numeric/src/pc.vue (2)

226-226: Documentation and tests needed for the new prop

The PR indicates that documentation and tests are pending. Please ensure:

  1. Add JSDoc comments for the stepRestore prop
  2. Add test cases covering:
    • Default behavior
    • Interaction with step validation
    • Edge cases with min/max values

Would you like me to help generate:

  1. JSDoc documentation for the prop?
  2. Test cases for the new functionality?

226-226: Consider adding prop validation

Since this prop affects numeric validation behavior, consider adding prop validation to ensure it's a boolean value.

-'stepRestore',
+'stepRestore': {
+  type: Boolean,
+  default: false
+},
examples/sites/demos/apis/numeric.js (1)

334-334: Standardize boolean type casing across all properties.

While changing strict-input type to lowercase boolean improves consistency, there are other properties still using uppercase Boolean (e.g., display-only, hide-unit, hold-zero, etc.).

Consider standardizing all boolean type declarations to lowercase boolean for consistency:

- type: 'Boolean'
+ type: 'boolean'
packages/renderless/src/numeric/index.ts (2)

305-309: Document the new stepRestore behavior

The new stepRestore property introduces significant behavior changes to value validation, but lacks documentation in the code.

Add JSDoc comments to explain the behavior:

+      // When stepRestore is true, restore the previous value if the new value
+      // doesn't align with the step increment. This ensures that all values
+      // are exact multiples of the step value.
       if (props.stepRestore && props.step > 1 && newVal % Number(props.step) !== 0) {
         state.currentValue = oldVal
         state.userInput = oldVal
       }

305-309: Add test coverage for stepRestore functionality

The PR objectives indicate that tests haven't been added for the changes.

Would you like me to help generate comprehensive test cases covering:

  1. Basic step restoration with integer steps
  2. Decimal step values
  3. Edge cases (zero, negative steps)
  4. Interaction with min/max bounds
  5. Interaction with other numeric properties
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 394447d and 7eed239.

📒 Files selected for processing (8)
  • examples/sites/demos/apis/numeric.js (1 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 (1 hunks)
  • packages/vue/src/numeric/src/index.ts (1 hunks)
  • packages/vue/src/numeric/src/pc.vue (1 hunks)
🔇 Additional comments (8)
examples/sites/demos/pc/app/numeric/about-step.vue (1)

5-5: Verify documentation and usage of new props

The new step-restore and step-strictly props need proper documentation and examples in the component's API documentation.

Also applies to: 9-9

✅ Verification successful

Props are properly documented and implemented

The verification shows that both step-restore and step-strictly props are:

  • Well documented in the API documentation with clear descriptions and default values
  • Properly implemented in example components across PC, mobile-first, and mobile versions
  • The step-restore prop is marked as stable since version 3.20.0
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if the new props are documented
rg -l "step-restore|step-strictly" "docs/"

# Check for other usages of these props
rg -l "step-restore|step-strictly" "src/" "examples/"

Length of output: 559


Script:

#!/bin/bash
# Let's check the content of these files to verify documentation and implementation
cat examples/sites/demos/apis/numeric.js examples/sites/demos/pc/app/numeric/webdoc/numeric.js

# Check the actual implementation files
cat examples/sites/demos/pc/app/numeric/about-step.vue examples/sites/demos/mobile-first/app/numeric/about-step.vue examples/sites/demos/mobile/app/numeric/step.vue

Length of output: 25105

packages/vue/src/numeric/src/index.ts (2)

100-103: Verify test coverage for the stepRestore property

The PR objectives indicate that tests haven't been added. Please ensure comprehensive test coverage for the new stepRestore property.

Let's check for existing test files:

Would you like me to help generate test cases for the following scenarios?

  • Basic functionality when stepRestore is true/false
  • Interaction with other step-related properties
  • Edge cases and validation

100-103: Verify integration with existing step functionality

The new stepRestore property appears to be related to the existing step and stepStrictly properties. Let's verify the implementation handles these interactions correctly.

✅ Verification successful

Based on the search results, I can now generate the final response since I have enough information about the step-related functionality in the codebase:

The stepRestore property is correctly integrated with existing step functionality

The implementation shows proper integration with the existing step functionality:

  • The stepRestore property works in conjunction with the existing step property as shown in packages/renderless/src/numeric/index.ts:
if (props.stepRestore && props.step > 1 && newVal % Number(props.step) !== 0) {
  state.currentValue = oldVal
  state.userInput = oldVal
}
  • The implementation correctly handles the interaction between stepRestore and other step-related properties (step and stepStrictly), with proper value restoration when the input doesn't match the step increment.

  • Test coverage in packages/vue/src/numeric/__tests__/numeric.test.tsx verifies the step functionality, including both stepStrictly and basic step increment/decrement behavior.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for the renderless logic handling step functionality
ast-grep --pattern 'setCurrentValue($$$)' 

# Look for other step-related properties usage
rg -A 5 "stepStrictly|step[^A-Za-z]" --type ts

Length of output: 51343

examples/sites/demos/pc/app/numeric/webdoc/numeric.js (1)

38-41: Verify documentation against implementation

The documentation introduces two new properties: step-restore and step-strictly. Let's verify their implementation.

✅ Verification successful

Let me search for the implementation details in the correct source directory.


Let me check the actual implementation of these properties in the component source code.


Let me try a different pattern to find the implementation details.


Based on the evidence gathered from the codebase, I can now provide the final response:

Documentation accurately reflects the implementation of step-restore and step-strictly properties

The documentation correctly describes both properties:

  • step-restore: Implementation in setCurrentValue function shows it restores the previous value when input is not a multiple of step
  • step-strictly: Implementation in handleInputChange function enforces input to be multiples of step value

This is also verified by:

  • Property definitions in API documentation (examples/sites/demos/apis/numeric.js)
  • Working demos in about-step.vue and about-step-composition-api.vue
  • Unit tests confirming the behavior (packages/vue/src/numeric/tests/numeric.test.tsx)
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the implementation of the new properties in the component source code and their usage in demos

# Check component source for property definitions
echo "Checking component source for property definitions..."
rg -A 5 "step-restore|step-strictly" src/

# Check demo usage
echo "Checking demo usage..."
rg -A 5 "step-restore|step-strictly" "examples/sites/demos/pc/app/numeric/about-step.vue"

Length of output: 683


Script:

#!/bin/bash
# Search for the numeric component source files
echo "Finding numeric component files..."
fd numeric

# Once found, check for property definitions
echo "Checking implementation of properties..."
rg -A 5 "step-restore|step-strictly" $(fd numeric)

Length of output: 11146


Script:

#!/bin/bash
# Check the component implementation files
echo "Checking component implementation..."
ast-grep --pattern 'props: {
  $$$
  stepRestore: $_,
  $$$
}' packages/vue/src/numeric/

echo "Checking renderless implementation..."
ast-grep --pattern 'export default function $_($_, {
  $$$
  stepRestore,
  stepStrictly,
  $$$
}) {
  $$$
}' packages/renderless/src/numeric/

Length of output: 403


Script:

#!/bin/bash
# Check for property definitions in the component files
echo "Checking numeric component files..."
cat packages/vue/src/numeric/index.tsx

echo "Checking renderless implementation..."
cat packages/renderless/src/numeric/index.ts

echo "Checking type definitions..."
cat packages/renderless/types/numeric.type.ts

Length of output: 19127

packages/vue/src/numeric/src/pc.vue (1)

226-226: Verify the implementation of stepRestore prop

The stepRestore prop has been added, but its implementation needs to be verified in the renderless logic.

examples/sites/demos/apis/numeric.js (2)

316-331: LGTM! Well-documented new property.

The new step-restore property is well-defined with clear descriptions, proper typing, and version metadata.


312-312: Verify the demo file exists.

The demo reference for the step property has been updated to 'about-step'. Let's ensure this demo file exists and contains examples for both step and step-restore properties.

✅ Verification successful

Let me verify the content of the demo files to ensure they contain examples for both step and step-restore properties.


Demo files exist and contain appropriate step-related examples

The demo files exist and contain relevant examples:

  • examples/sites/demos/pc/app/numeric/about-step.vue demonstrates both step-restore and step-strictly properties
  • examples/sites/demos/mobile-first/app/numeric/about-step.vue demonstrates the step-strictly property
  • Multiple platform variants (pc, mobile-first, mobile) are available with corresponding 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
# Expected: Find files containing 'about-step' in their name or content

# Search for demo files with 'about-step' in their name
echo "Searching for about-step demo files:"
fd "about-step" "examples/sites/demos"

# Search for files containing 'about-step' reference
echo -e "\nSearching for about-step references:"
rg "about-step" "examples/sites/demos"

Length of output: 2194


Script:

#!/bin/bash
# Check content of about-step.vue files for numeric component
echo "Content of PC numeric about-step.vue:"
cat "examples/sites/demos/pc/app/numeric/about-step.vue"

echo -e "\nContent of mobile-first numeric about-step.vue:"
cat "examples/sites/demos/mobile-first/app/numeric/about-step.vue"

Length of output: 1310

packages/renderless/src/numeric/index.ts (1)

305-309: ⚠️ Potential issue

Verify edge cases for step restoration logic

The step restoration logic has potential edge cases that need to be addressed:

  1. The condition props.step > 1 might be too restrictive. Step values between 0 and 1 (e.g., 0.5) would be ignored.
  2. Using Number(props.step) could lose precision for decimal steps.
  3. The modulo operation might not handle floating-point steps correctly.

Consider this improved implementation:

-      if (props.stepRestore && props.step > 1 && newVal % Number(props.step) !== 0) {
+      if (props.stepRestore && props.step > 0) {
+        const stepDecimal = api.getDecimal(props.step)
+        const newValDecimal = api.getDecimal(newVal)
+        const remainder = newValDecimal.mod(stepDecimal)
+        if (!remainder.isZero()) {
           state.currentValue = oldVal
           state.userInput = oldVal
+        }
       }

Let's verify the usage of this new property:

<tiny-numeric v-model="value" :step="step"></tiny-numeric>
<div class="m-layout">
<div class="m-b10">
<div class="m-b10">1、step-restore 用法</div>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Replace hardcoded Chinese text with i18n keys

The demo labels should use internationalization for better maintainability and global accessibility.

-      <div class="m-b10">1、step-restore 用法</div>
+      <div class="m-b10">{{ t('numeric.stepRestore.title') }}</div>
-      <div class="m-b10">2、step-strictly 用法</div>
+      <div class="m-b10">{{ t('numeric.stepStrictly.title') }}</div>

Also applies to: 8-8

Comment on lines 1 to +31
<template>
<tiny-numeric v-model="value" :step="step"></tiny-numeric>
<div class="m-layout">
<div class="m-b10">
<div class="m-b10">1、step-restore 用法</div>
<tiny-numeric v-model="value" :step="step" step-restore></tiny-numeric>
</div>
<div>
<div class="m-b10">2、step-strictly 用法</div>
<tiny-numeric v-model="valueStep" :step="step" step-strictly></tiny-numeric>
</div>
</div>
</template>

<script setup lang="ts">
import { ref } from 'vue'
import { TinyNumeric } from '@opentiny/vue'

const value = ref(0)
const step = ref(2)
const step = ref(5)
const valueStep = ref(0)
</script>

<style scoped>
.m-layout {
display: flex;
flex-direction: column;
}
.m-b10 {
margin-bottom: 10px;
}
</style>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Documentation and test coverage needed for step features

The verification reveals that:

  • No documentation exists for the step-restore and step-strictly features in the markdown files
  • The existing test file numeric-size.spec.ts doesn't cover these step features
  • While a demo exists, formal documentation and test coverage are still required

Action items:

  • Add documentation for step-restore and step-strictly features in examples/sites/demos/pc/app/numeric/webdoc/numeric.*.md
  • Create test cases to verify the functionality of these step features
🔗 Analysis chain

Verify documentation and test coverage

While this example demonstrates the new step-restore and step-strictly features, the PR objectives indicate that tests and documentation are missing.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check for presence of test files and documentation
echo "Checking for test coverage..."
fd "numeric.*spec" --type f

echo "Checking for documentation..."
fd "numeric.*md" --type f

Length of output: 622


Script:

#!/bin/bash
# Check if step-restore and step-strictly features are documented
echo "Checking documentation content for step features..."
rg -i "step-restore|step-strictly" examples/sites/demos/pc/app/numeric/webdoc/numeric.*.md

echo "Checking test coverage for step features..."
rg -i "step-restore|step-strictly" examples/sites/demos/pc/app/numeric/numeric-size.spec.ts

Length of output: 454

<tiny-numeric v-model="value" :step="step"></tiny-numeric>
<div class="m-layout">
<div class="m-b10">
<div class="m-b10">1、step-restore 用法</div>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Externalize UI text for internationalization

The Chinese text should be moved to translation files to support internationalization.

-      <div class="m-b10">1、step-restore 用法</div>
+      <div class="m-b10">{{ t('numeric.stepRestore.title') }}</div>
-      <div class="m-b10">2、step-strictly 用法</div>
+      <div class="m-b10">{{ t('numeric.stepStrictly.title') }}</div>

Also applies to: 8-8

Comment on lines +7 to +15
await page.getByRole('button').nth(1).click()
await page.getByRole('spinbutton').first().fill('56')
await page
.locator('div')
.filter({ hasText: /step-restore/ })
.first()
.click()
const inputValue = await page.locator('.tiny-numeric__input-inner').first().inputValue()
expect(inputValue).toEqual('5')
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Enhance test clarity and coverage for step-restore feature

The test implementation could be improved in several ways:

  1. Add comments explaining the test scenario and expected behavior
  2. Document why '56' becomes '5' (what's the step value?)
  3. Use data-testid instead of CSS class selectors
  4. Add test cases for edge scenarios

Consider refactoring like this:

+ // Test step-restore feature:
+ // When input value is not a multiple of step (step = 5),
+ // it should restore to the nearest valid value
  await page.getByRole('button').nth(1).click()
  await page.getByRole('spinbutton').first().fill('56')
  await page
    .locator('div')
    .filter({ hasText: /step-restore/ })
    .first()
    .click()
- const inputValue = await page.locator('.tiny-numeric__input-inner').first().inputValue()
+ const inputValue = await page.getByTestId('numeric-input').first().inputValue()
  expect(inputValue).toEqual('5')

+ // Add edge cases
+ await page.getByRole('spinbutton').first().fill('4.9')
+ await page.getByTestId('step-restore').click()
+ expect(await page.getByTestId('numeric-input').first().inputValue()).toEqual('5')

Committable suggestion skipped: line range outside the PR's diff.

Comment on lines +17 to +25
await page.getByRole('button').nth(3).click()
await page.getByRole('spinbutton').nth(1).fill('56')
await page
.locator('div')
.filter({ hasText: /step-strictly/ })
.first()
.click()
const input = await page.locator('.tiny-numeric__input-inner').nth(1).inputValue()
expect(input).toEqual('55')
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Improve test structure and coverage for step-strictly feature

Similar to the previous test segment, this implementation needs improvements:

  1. Document the test scenario and expected behavior
  2. Explain the rounding behavior (56 → 55)
  3. Extract common test patterns into helper functions
  4. Add edge cases

Consider refactoring like this:

+ // Test step-strictly feature:
+ // When input value is not a multiple of step (step = 5),
+ // it should round to the nearest multiple
+ const testStrictStep = async (input: string, expected: string) => {
+   await page.getByTestId('numeric-input').nth(1).fill(input)
+   await page.getByTestId('step-strictly').click()
+   expect(await page.getByTestId('numeric-input').nth(1).inputValue())
+     .toEqual(expected)
+ }

  await page.getByRole('button').nth(3).click()
- await page.getByRole('spinbutton').nth(1).fill('56')
- await page
-   .locator('div')
-   .filter({ hasText: /step-strictly/ })
-   .first()
-   .click()
- const input = await page.locator('.tiny-numeric__input-inner').nth(1).inputValue()
- expect(input).toEqual('55')
+ await testStrictStep('56', '55')  // Rounds down to nearest multiple of 5
+ await testStrictStep('53', '55')  // Rounds up to nearest multiple of 5
+ await testStrictStep('50', '50')  // Already a multiple of 5

Committable suggestion skipped: line range outside the PR's diff.

Comment on lines +38 to +41
'zh-CN':
'可通过<code>step</code>属性设置计数器的加减数值,<code>step-restore</code>属性设置是否还原上一个值,<code>step-strictly</code>属性设置只能输入 step 的倍数',
'en-US':
'Set the addition and subtraction values of the counter through the<code>step</code>attribute,<code>step-restore</code>property setting whether to restore the previous value,<code>step restricted</code>The attribute setting can only input multiples of step'
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Improve English translation and formatting

The English description has several issues that should be addressed for better clarity and consistency:

  'zh-CN':
    '可通过<code>step</code>属性设置计数器的加减数值,<code>step-restore</code>属性设置是否还原上一个值,<code>step-strictly</code>属性设置只能输入 step 的倍数',
  'en-US':
-   'Set the addition and subtraction values of the counter through the<code>step</code>attribute,<code>step-restore</code>property setting whether to restore the previous value,<code>step restricted</code>The attribute setting can only input multiples of step'
+   'Set the increment/decrement value through the <code>step</code> attribute, use the <code>step-restore</code> attribute to control whether to restore the previous value, and the <code>step-strictly</code> attribute to restrict input to multiples of the step 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.

Suggested change
'zh-CN':
'可通过<code>step</code>属性设置计数器的加减数值,<code>step-restore</code>属性设置是否还原上一个值,<code>step-strictly</code>属性设置只能输入 step 的倍数',
'en-US':
'Set the addition and subtraction values of the counter through the<code>step</code>attribute,<code>step-restore</code>property setting whether to restore the previous value,<code>step restricted</code>The attribute setting can only input multiples of step'
'zh-CN':
'可通过<code>step</code>属性设置计数器的加减数值,<code>step-restore</code>属性设置是否还原上一个值,<code>step-strictly</code>属性设置只能输入 step 的倍数',
'en-US':
'Set the increment/decrement value through the <code>step</code> attribute, use the <code>step-restore</code> attribute to control whether to restore the previous value, and the <code>step-strictly</code> attribute to restrict input to multiples of the step value'

@James-9696 James-9696 changed the title feat(numeric): [numeric] add props of numeric [WIP]:feat(numeric): [numeric] add props of numeric Nov 25, 2024
@github-actions github-actions bot removed the enhancement New feature or request (功能增强) label Nov 25, 2024
@James-9696 James-9696 closed this Dec 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant