feat(form): [form] form component adapt to old theme #2562
Conversation
WalkthroughThis pull request introduces several changes across multiple files, primarily focusing on the Changes
Possibly related PRs
Suggested labels
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: 3
🧹 Outside diff range and nitpick comments (13)
examples/sites/demos/pc/app/config-provider/form.spec.ts (3)
3-6: Consider improving test maintainability and reliabilityA few suggestions to enhance the test setup:
- Use English for test names to improve maintainability across teams
- Add more specific error handling
- Ensure page is fully loaded after navigation
-test('测试隐藏星号', async ({ page }) => { +test('should hide required asterisk in form label', async ({ page }) => { page.on('pageerror', (exception) => expect(exception).toBeNull()) await page.goto('config-provider#form') + // Wait for network to be idle to ensure full page load + await page.waitForLoadState('networkidle')
7-7: Enhance visibility check robustnessThe current visibility check could be more comprehensive to ensure the form is fully rendered and stable.
- await expect(page.locator('#form .tiny-form')).toBeVisible() + const formElement = page.locator('#form .tiny-form') + await expect(formElement).toBeVisible({ timeout: 5000 }) + // Ensure form is rendered with expected structure + await expect(formElement.locator('.tiny-form-item')).toBeVisible()
1-14: Consider improving test architectureTo enhance the test suite's maintainability and reliability, consider:
- Creating shared test fixtures for common form testing scenarios
- Adding test isolation through proper setup/teardown
- Implementing page object pattern for form interactions
Example implementation:
// formPage.ts class FormPage { constructor(private page: Page) {} async navigate() { await this.page.goto('config-provider#form') await this.page.waitForLoadState('networkidle') } async getRequiredAsteriskStyle() { // ... style checking logic } } // form.spec.ts test.describe('Form Component', () => { let formPage: FormPage; test.beforeEach(async ({ page }) => { formPage = new FormPage(page); await formPage.navigate(); }); test('should hide required asterisk', async () => { // ... test using formPage methods }); });examples/sites/demos/pc/app/config-provider/form-composition-api.vue (3)
1-14: LGTM! Consider adding ARIA labels for better accessibility.The template structure is well-organized and follows Vue 3 composition API best practices.
Consider adding aria-labels to improve accessibility:
- <tiny-numeric v-model="formData.age"></tiny-numeric> + <tiny-numeric v-model="formData.age" aria-label="年龄输入框"></tiny-numeric> - <tiny-input v-model="formData.name"></tiny-input> + <tiny-input v-model="formData.name" aria-label="姓名输入框"></tiny-input>
30-33: Consider adding TypeScript interface for form data.The form data initialization is correct, but could benefit from type safety.
Consider adding this interface above the initialization:
interface FormData { name: string; age: string; } const formData = ref<FormData>({ name: '', age: '' })
36-40: Consider using responsive width.While the current fixed width works, consider making it responsive for better mobile support.
.demo-form { - width: 380px; + width: 100%; + max-width: 380px; }examples/sites/demos/pc/app/form/hide-required.spec.ts (1)
Line range hint
1-33: Enhance test robustness and maintainability.While the test logic is sound, consider these improvements:
- Use English for test descriptions to improve international maintainability
- Add initial state verification before clicking the switch
- Extract magic values into constants
Here's a suggested refactor:
import { test, expect } from '@playwright/test' + +const REQUIRED_MARKER = '"*"' +const SELECTOR = { + demo: '#hide-required', + label: '.tiny-form-item__label', + form: '.tiny-form', + switch: '.tiny-switch' +} -test('必填项红色星号', async ({ page }) => { +test('should toggle required field asterisk visibility', async ({ page }) => { page.on('pageerror', (exception) => expect(exception).toBeNull()) - await page.goto('form#hide-required') + await page.goto(`form${SELECTOR.demo}`) - const demo = page.locator('#hide-required') - const form = demo.locator('.tiny-form') - const firstLabel = form.locator('.tiny-form-item__label').first() - const switchBtn = demo.locator('.tiny-switch') + const demo = page.locator(SELECTOR.demo) + const form = demo.locator(SELECTOR.form) + const firstLabel = form.locator(SELECTOR.label).first() + const switchBtn = demo.locator(SELECTOR.switch) + + // Verify initial state + let initialState = await page.evaluate(() => { + const labelBefore = document.querySelector('.tiny-form .tiny-form-item__label') + const { display, content } = window.getComputedStyle(labelBefore, '::before') + return { display, content } + }) + expect(initialState.display).toBe('none') await switchBtn.click() await expect(firstLabel).toBeVisible() let beforeElement = await page.evaluate(() => { const labelBefore = document.querySelector('.tiny-form .tiny-form-item__label') const { display, content } = window.getComputedStyle(labelBefore, '::before') return { display, content } }) - expect(beforeElement.content).toBe('"*"') + expect(beforeElement.content).toBe(REQUIRED_MARKER)examples/sites/demos/pc/app/config-provider/form.vue (3)
1-14: Consider enhancing internationalization and accessibilityThe template could be improved in the following areas:
- Use i18n for labels instead of hardcoded Chinese text
- Add aria-labels and validation messages for better accessibility
Here's a suggested improvement:
<template> <div> <tiny-config-provider :design="design"> <tiny-form :model="formData" class="demo-form"> - <tiny-form-item label="年龄" prop="age" required> + <tiny-form-item + :label="t('form.age')" + prop="age" + required + :validate-message="t('form.age.validation')"> <tiny-numeric v-model="formData.age"></tiny-numeric> </tiny-form-item> - <tiny-form-item label="姓名" prop="name" required> + <tiny-form-item + :label="t('form.name')" + prop="name" + required + :validate-message="t('form.name.validation')"> <tiny-input v-model="formData.name"></tiny-input> </tiny-form-item> </tiny-form> </tiny-config-provider> </div> </template>
29-37: Enhance configuration demo and documentation
- Comments should be in English for better maintainability
- Consider showcasing more config-provider capabilities in this demo
design: { - name: 'smb', // 设计规范名称 - version: '1.0.0', // 设计规范版本号 + name: 'smb', // design specification name + version: '1.0.0', // design specification version components: { Form: { - hideRequiredAsterisk: true + hideRequiredAsterisk: true, + // Consider demonstrating other form configurations: + // labelPosition: 'top', + // labelWidth: '120px', + // size: 'small' } }
47-51: Consider using responsive widthThe fixed width of 380px might not be optimal for all screen sizes. Consider using relative units or media queries for better responsiveness.
<style scoped> .demo-form { - width: 380px; + width: 100%; + max-width: 380px; } </style>packages/renderless/src/form/index.ts (2)
Line range hint
236-242: Improve error message in getLabelWidthIndexThe error message could be more descriptive to help with debugging.
- throw new Error('unpected width ', width as ErrorOptions) + throw new Error(`Unexpected label width value: ${width}. This width was not registered with the form.`)
Line range hint
277-307: Consider improving type safety in bindDialogEventThe dialog event handlers could benefit from stronger typing:
- The
boxCloseHandlerparameter type should be explicitly defined- The dialog emitter events could use a type-safe event system
- const boxCloseHandler = (isFormReset = true) => { + const boxCloseHandler = (isFormReset: boolean = true) => { - dialog.state.emitter.on('boxclose', boxCloseHandler) + type DialogEvents = { + boxclose: (isFormReset: boolean) => void; + boxdrag: () => void; + }; + (dialog.state.emitter as TypedEmitter<DialogEvents>).on('boxclose', boxCloseHandler)examples/sites/demos/apis/form.js (1)
Line range hint
365-371: Type Inconsistency: Use lowercase 'boolean' instead of 'Boolean'The type for
validate-tagis specified as 'Boolean' which is inconsistent with TypeScript conventions. For better type consistency across the codebase, use lowercase 'boolean'.Apply this change:
- type: 'Boolean', + type: 'boolean',
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (13)
examples/sites/demos/apis/form.js(1 hunks)examples/sites/demos/pc/app/config-provider/form-composition-api.vue(1 hunks)examples/sites/demos/pc/app/config-provider/form.spec.ts(1 hunks)examples/sites/demos/pc/app/config-provider/form.vue(1 hunks)examples/sites/demos/pc/app/config-provider/webdoc/config-provider.js(1 hunks)examples/sites/demos/pc/app/form/hide-required.spec.ts(1 hunks)examples/sites/demos/pc/app/form/slot-label-composition-api.vue(1 hunks)examples/sites/demos/pc/app/form/slot-label.spec.ts(1 hunks)examples/sites/demos/pc/app/form/slot-label.vue(1 hunks)examples/sites/demos/pc/app/form/webdoc/form.js(1 hunks)packages/design/aurora/src/form/index.ts(0 hunks)packages/design/saas/src/form/index.ts(0 hunks)packages/renderless/src/form/index.ts(1 hunks)
💤 Files with no reviewable changes (2)
- packages/design/aurora/src/form/index.ts
- packages/design/saas/src/form/index.ts
✅ Files skipped from review due to trivial changes (2)
- examples/sites/demos/pc/app/form/slot-label-composition-api.vue
- examples/sites/demos/pc/app/form/slot-label.vue
🔇 Additional comments (10)
examples/sites/demos/pc/app/form/slot-label.spec.ts (1)
13-13: LGTM! Verify text consistency across related files.
The text change improves clarity by explicitly mentioning the ellipsis behavior.
Let's verify that this text change is consistent across related files:
✅ Verification successful
Text change is consistently applied across the codebase
The verification shows that:
- The old text "超过两行文字，省略显示" is not found anywhere in the codebase
- The new text "超过两行文字，显示省略号" is consistently used in:
- examples/sites/demos/pc/app/form/slot-label-composition-api.vue
- examples/sites/demos/pc/app/form/slot-label.vue
- examples/sites/demos/pc/app/form/slot-label.spec.ts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistency of the label text across related files
# Expected: The new text should be used consistently
# Search for both old and new text variations to ensure complete replacement
echo "Checking for old text..."
rg "超过两行文字，省略显示" -A 2 -B 2
echo "Checking for new text..."
rg "超过两行文字，显示省略号" -A 2 -B 2
Length of output: 2013
examples/sites/demos/pc/app/config-provider/form-composition-api.vue (2)
16-18: LGTM! Imports are well-structured.
The imports are properly organized using named imports, which is optimal for tree-shaking.
20-28: Verify design system configuration alignment with PR objectives.
The PR aims to adapt to an old theme, but the configuration needs clarification:
- The design system name 'smb' should be documented
- The version number '1.0.0' seems generic
- Chinese comment should be translated for international collaboration
const design = ref({
- name: 'smb', // 设计规范名称
- version: '1.0.0', // 设计规范版本号
+ name: 'smb', // Design specification name
+ version: '1.0.0', // Design specification version
components: {
Form: {
hideRequiredAsterisk: true
}
}
})Run this script to verify the design system configuration:
✅ Verification successful
The previous command failed because the file type wasn't recognized. Let's try broader searches to understand the design system configuration context and documentation.
Let's examine the theme documentation and related files to understand if 'smb' is a valid design system name and gather context about version numbering conventions.
Design configuration is correctly aligned with component library standards
The configuration is valid and follows the established patterns:
- The 'smb' design name is a recognized theme variant documented in the codebase
- The version '1.0.0' follows semantic versioning
- The
hideRequiredAsteriskconfiguration is a supported Form component option
The Chinese comments can be translated but are not critical since they are development-time documentation only.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other instances of smb theme configuration
rg -l "name: ['|\"]smb['|\"]" --type vue
Length of output: 74
Script:
#!/bin/bash
# Search for smb theme configuration with broader pattern and without file type restriction
rg -l "['|\"]smb['|\"]"
# Search for design system configuration patterns
rg -l "design.*=.*ref"
# Look for documentation about design systems
fd "README|CONTRIBUTING" --type f --exec grep -l "design" {} \;
Length of output: 1272
Script:
#!/bin/bash
# Check the theme documentation for SMB design system details
cat examples/sites/demos/pc/webdoc/theme.md examples/sites/demos/pc/webdoc/theme-en.md
# Look at other config-provider examples to understand the design system usage
cat examples/sites/demos/pc/app/config-provider/base.vue examples/sites/demos/pc/app/config-provider/form.vue
# Check theme-related README files for design system documentation
cat packages/theme/README.md packages/theme-saas/README.md
Length of output: 30168
examples/sites/demos/pc/app/config-provider/form.vue (2)
17-26: LGTM!
Component imports and registration are properly implemented following Vue.js best practices.
29-32: Verify theme configuration
Please confirm if 'smb' with version '1.0.0' is the correct old theme mentioned in the PR objectives.
✅ Verification successful
Based on the search results, I can see extensive evidence that 'smb' is a legitimate theme in the codebase:
- It's one of the main themes in the project, alongside others like 'aurora' and 'infinity'
- There are numerous PRs and changes related to adapting components to the 'smb' theme
- The theme configuration shows 'smb' is a valid theme name with version '1.0.0'
- The codebase includes dedicated SMB theme files and configurations
Theme configuration is correct
The use of 'smb' theme with version '1.0.0' in the form component aligns with the codebase's theme system and appears to be the intended configuration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for theme-related configurations and documentation
rg -g '!node_modules' -g '!dist' -A 5 'smb.*theme|theme.*smb'
# Search for other theme configurations in the codebase
ast-grep --pattern 'design: {
name: $_,
version: $_
}'
Length of output: 33944
examples/sites/demos/pc/app/config-provider/webdoc/config-provider.js (2)
43-54: LGTM! Demo entry structure follows the established pattern.
The new form demo entry is well-structured with proper internationalization and consistent formatting.
51-52: Please enhance the description with more details about the design prop usage.
The description mentions using the design prop but doesn't specify:
- What value should be set for the
designprop to achieve this behavior - How this aligns with the default behavior of showing asterisks (as mentioned in the AI summary, the default for
hide-required-asteriskis now 'false')
Let's verify the actual implementation in the demo file:
✅ Verification successful
Based on the shell script results, I found the implementation details in one of the form demo files that shows exactly how the design prop should be used. I can now generate the final response:
The design prop should be configured with a specific structure to hide required asterisks
The design prop should be set with the following structure:
{
name: 'smb', // design spec name
version: '1.0.0', // design spec version
components: {
Form: {
hideRequiredAsterisk: true
}
}
}This aligns with the default behavior where asterisks are shown by default (hideRequiredAsterisk: false), and you can explicitly set hideRequiredAsterisk: true to hide them.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the implementation details in the form demo file
# Expected: Find the design prop usage and its value
# Look for the form demo file
fd "form.vue" --exec cat {} \;
# Search for ConfigProvider usage with design prop
ast-grep --pattern 'ConfigProvider :design="$_"'
Length of output: 15619
examples/sites/demos/pc/app/form/webdoc/form.js (2)
266-277: LGTM! Improved naming clarity.
The renaming from smb-required to hide-required better reflects the functionality and maintains consistency across the demo configuration.
273-273: Verify impact of default value change.
The change in default value of hide-required-asterisk from true to false is a potentially breaking change that could affect existing implementations. This change means required fields will now show asterisks by default.
Also applies to: 275-275
examples/sites/demos/apis/form.js (1)
47-47:
Breaking Change: Default visibility of required field asterisks has been reversed
The default value of hide-required-asterisk has been changed from 'true' to 'false', which means required field indicators (asterisks) will now be visible by default. While this aligns better with common form UX patterns, it's a breaking change that could affect existing implementations.
Let's verify the impact on existing form implementations:
| const beforeElement = await page.evaluate(() => { | ||
| const labelBefore = document.querySelector('.tiny-form .tiny-form-item__label') | ||
| const { display, content } = window.getComputedStyle(labelBefore, '::before') | ||
| return { display, content } | ||
| }) | ||
| expect(beforeElement.content).toBe('none') |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve style evaluation reliability and clarity
The current style evaluation could be more robust and self-documenting:
- Add error handling for missing elements
- Make assertions more specific
- Add comments explaining the expected behavior
const beforeElement = await page.evaluate(() => {
- const labelBefore = document.querySelector('.tiny-form .tiny-form-item__label')
+ const labelBefore = document.querySelector('.tiny-form .tiny-form-item__label');
+ if (!labelBefore) {
+ throw new Error('Form label element not found');
+ }
const { display, content } = window.getComputedStyle(labelBefore, '::before')
return { display, content }
})
- expect(beforeElement.content).toBe('none')
+ // Verify that the required asterisk is hidden
+ expect(beforeElement.content, 'Required asterisk should be hidden').toBe('none')📝 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 beforeElement = await page.evaluate(() => { | |
| const labelBefore = document.querySelector('.tiny-form .tiny-form-item__label') | |
| const { display, content } = window.getComputedStyle(labelBefore, '::before') | |
| return { display, content } | |
| }) | |
| expect(beforeElement.content).toBe('none') | |
| const beforeElement = await page.evaluate(() => { | |
| const labelBefore = document.querySelector('.tiny-form .tiny-form-item__label'); | |
| if (!labelBefore) { | |
| throw new Error('Form label element not found'); | |
| } | |
| const { display, content } = window.getComputedStyle(labelBefore, '::before') | |
| return { display, content } | |
| }) | |
| // Verify that the required asterisk is hidden | |
| expect(beforeElement.content, 'Required asterisk should be hidden').toBe('none') |
| await page.goto('form#hide-required') | ||
|
|
||
| const demo = page.locator('#smb-required') | ||
| const demo = page.locator('#hide-required') |
There was a problem hiding this comment.
💡 Codebase verification
Inconsistency found in demo ID configuration
The test file has been updated to use 'hide-required', but there's an inconsistency in the configuration:
examples/sites/demos/apis/form.jsstill referencessmb-required- The demo ID 'hide-required' is properly configured in
examples/sites/demos/pc/app/form/webdoc/form.js
This mismatch needs to be fixed to maintain consistency across the codebase.
🔗 Analysis chain
LGTM! Verify the demo ID changes across related files.
The navigation and selector updates are consistent with the renamed demo ID.
Let's verify the demo ID changes are consistent across related files:
The previous results show that 'smb-required' is still present in apis/form.js while the test was updated to use 'hide-required'. Let's verify all occurrences and related files to ensure consistency.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all references to the old demo ID 'smb-required' have been updated to 'hide-required'
# Search for any remaining references to the old ID
echo "Checking for any remaining references to 'smb-required'..."
rg "smb-required"
# Search for the new ID to confirm it exists in demo configuration
echo "Verifying 'hide-required' exists in demo files..."
rg "hide-required" "examples/sites/demos/webdoc/form.js"
Length of output: 464
Script:
#!/bin/bash
# Let's check all possible demo configuration files and related files
echo "Checking for 'hide-required' in all files..."
rg "hide-required"
echo -e "\nChecking form demo configuration files..."
fd "form.js" examples/sites/demos/
Length of output: 2455
| @@ -47,7 +47,7 @@ export const computedAutoLabelWidth = | |||
| export const computedHideRequiredAsterisk = | |||
| ({ props, designConfig }: Pick<IFormRenderlessParams, 'props' | 'designConfig'>) => | |||
| (): boolean => { | |||
| return props.hideRequiredAsterisk ?? designConfig?.hideRequiredAsterisk ?? true | |||
| return props.hideRequiredAsterisk ?? designConfig?.hideRequiredAsterisk ?? false | |||
There was a problem hiding this comment.
💡 Codebase verification
Breaking Change: Default behavior change requires attention
The demo files confirm this is a breaking change. Both the Options API and Composition API examples explicitly set hideRequiredAsterisk: true as their default state, indicating this was the expected behavior. Changing the default from true to false will affect any forms that rely on the default behavior.
Action items:
- This change needs to be documented in the changelog as a breaking change
- Consider implementing one of these solutions:
- Add a version-specific fallback
- Create a migration guide
- Revert to the original default and implement the new behavior behind a feature flag
🔗 Analysis chain
Breaking Change: Required asterisk visibility default behavior has changed
The default behavior for hiding required asterisks has been inverted from true to false. This means required fields will now show asterisks by default, which could affect the appearance of existing forms.
Let's verify the impact on existing forms and documentation:
Action items:
- Consider adding a migration guide for this breaking change
- Update component documentation to reflect the new default behavior
- Consider adding a feature flag or version-specific fallback for backward compatibility
The previous results show extensive usage of hideRequiredAsterisk across the codebase, including demo files. Let's verify the actual implementation and documented behavior to confirm if this is truly a breaking change and understand its impact.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for form usages and documentation that might need updates
# Check for form component usage in templates
rg -t vue "v-tiny-form|tiny-form" -A 5
# Check for hideRequiredAsterisk property usage
rg "hideRequiredAsterisk"
# Check documentation files
fd -e md -e mdx | xargs rg "hideRequiredAsterisk|required asterisk"
Length of output: 3520
Script:
#!/bin/bash
# Let's check the git history to understand if this was indeed a breaking change
git log -L :computedHideRequiredAsterisk:packages/renderless/src/form/index.ts
# Check for any tests related to this functionality
rg -l "hideRequiredAsterisk.*test"
# Check the demo files to understand the documented behavior
cat examples/sites/demos/pc/app/form/hide-required.vue
cat examples/sites/demos/pc/app/form/hide-required-composition-api.vue
Length of output: 3597
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
Bug Fixes
Tests
Documentation