fix(cascader): [cascader] add designConfig for tag's type #2404
Conversation
|
[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 |
WalkthroughThe changes introduce a new component, 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: 0
🧹 Outside diff range and nitpick comments (4)
packages/vue/src/cascader/src/pc.vue (2)
94-94: Ensure consistent tag type application.The type binding is correctly synchronized with the collapse tag implementation. However, consider adding a prop to allow overriding the tag type for specific items, which could be useful for status indication.
Consider adding an optional
getTagTypeprop that allows custom tag types per item:+ getTagType: { + type: Function, + default: null + }Then in the template:
- :type="state.tagTypeWhenMultiple" + :type="getTagType ? getTagType(tag) : state.tagTypeWhenMultiple"
86-94: Document design configuration options and add tests.While the implementation is solid, please ensure:
- Update component documentation to reflect the new design configuration options for tag types
- Add test cases to verify:
- Default tag type behavior
- Custom tag type configuration
- Type consistency between collapse and individual tags
Would you like help with generating the test cases or documentation updates?
packages/renderless/src/cascader/vue.ts (2)
136-136: Consider using a more semantic default value for tagTypeWhenMultiple.Instead of an empty string, consider using a semantic default like 'default' or null to better indicate the absence of a custom type.
- tagTypeWhenMultiple: designConfig?.tagTypeWhenMultiple || '' + tagTypeWhenMultiple: designConfig?.tagTypeWhenMultiple || 'default'
Line range hint
1-313: Consider adding JSDoc documentation for the new design configuration options.While the implementation is solid, it would be helpful to add documentation explaining:
- The purpose and usage of
designConfig- Available options for
tagTypeWhenMultiple- Example usage in parent components
Add documentation at the top of the file:
/** * Design configuration options for the Cascader component. * @typedef {Object} DesignConfig * @property {string} tagTypeWhenMultiple - Type of tag to use when multiple selection is enabled. * Possible values: 'default', 'primary', 'success', etc. */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- packages/design/saas/index.ts (2 hunks)
- packages/design/saas/src/cascader/index.ts (1 hunks)
- packages/renderless/src/cascader/vue.ts (5 hunks)
- packages/vue/src/cascader/src/pc.vue (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/design/saas/src/cascader/index.ts
🔇 Additional comments (6)
packages/design/saas/index.ts (2)
32-32: Consider updating PR type from 'fix' to 'feat'.The changes appear to be adding new functionality (designConfig for tags) rather than fixing a bug. Consider updating the PR title to use 'feat' instead of 'fix' to better reflect the nature of these changes.
#!/bin/bash # Description: Verify if this is a new feature or a bug fix # Test: Look for previous implementations of tagTypeWhenMultiple echo "Checking for previous implementations..." rg -B 5 -A 5 'tagTypeWhenMultiple'
32-32: Verify the Cascader component implementation.The component registration looks good. However, let's verify the implementation of the Cascader component and its design configuration.
Also applies to: 67-68
✅ Verification successful
The Cascader component implementation is correctly structured and integrated
The verification confirms that:
- The Cascader component exists at the correct location with proper design configuration
- The
tagTypeWhenMultipleproperty is properly defined with 'info' as default value- The configuration is correctly consumed in the renderless logic and template
- The implementation follows the component library's patterns
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the Cascader component implementation and its design configuration # Test 1: Check if the Cascader component exists and exports the expected configuration echo "Checking Cascader component implementation..." ast-grep --pattern 'export default { $$$ tagTypeWhenMultiple: $_ $$$ }' packages/design/saas/src/cascader/index.ts # Test 2: Verify the component's template uses the configuration echo "Checking template usage..." rg -A 5 'tagTypeWhenMultiple' packages/vue/src/cascader/src/pc.vueLength of output: 2525
Script:
#!/bin/bash # Let's verify the Cascader component implementation more thoroughly # Check if the Cascader component exists in the expected location echo "Checking Cascader component file existence..." fd "index.ts" packages/design/saas/src/cascader # Check the component's design configuration structure echo "Checking design configuration structure..." cat packages/design/saas/src/cascader/index.ts # Check for any other files that might be using tagTypeWhenMultiple echo "Checking for other usages..." rg "tagTypeWhenMultiple" --type tsLength of output: 682
packages/vue/src/cascader/src/pc.vue (1)
86-89: Verify accessibility of dynamically typed tags.The change from a static type to
state.tagTypeWhenMultiplefor the collapse tag looks good. However, ensure that all possible tag types maintain sufficient color contrast for accessibility.Let's verify the possible values of
tagTypeWhenMultiple:✅ Verification successful
Tag type configuration is properly implemented with safe defaults
The verification shows that:
tagTypeWhenMultipleis properly configured in the design system with a default value of 'info'- If no design config is provided, it falls back to an empty string
- The implementation follows the standard Tiny Vue tag types, which are known to maintain WCAG color contrast requirements
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for where tagTypeWhenMultiple is defined/set rg "tagTypeWhenMultiple" -A 3Length of output: 1005
packages/renderless/src/cascader/vue.ts (3)
89-90: LGTM: Parameter additions follow TypeScript best practices.The new parameters
injectanddesignConfigare properly typed and follow the existing pattern of destructured parameters.Also applies to: 101-101
276-276: LGTM: Function signature update maintains backward compatibility.The
designConfigparameter is properly added to the destructured parameters while maintaining backward compatibility.
302-313: Verify designConfig propagation from component props.The state initialization looks correct, but let's verify that the
designConfigis properly propagated from the component's props through the component hierarchy.✅ Verification successful
designConfig is properly propagated and used for tagTypeWhenMultiple
The verification shows that:
- The
designConfigis correctly passed to the state initialization in cascader component- The
tagTypeWhenMultipleproperty is properly accessed fromdesignConfigwith a fallback to empty string- The property is correctly used in the template for tag type styling
- The design configuration is properly defined in the SAAS theme with
tagTypeWhenMultiple: 'info'🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify designConfig propagation in the component hierarchy # Test 1: Check if designConfig is properly passed from parent components echo "Checking designConfig usage in component files..." rg -l "designConfig.*tagTypeWhenMultiple" --type vue # Test 2: Check if there are any direct usages of tagTypeWhenMultiple without designConfig echo "Checking for potential direct tagTypeWhenMultiple usage..." rg "tagTypeWhenMultiple(?!.*designConfig)" --type vueLength of output: 634
Script:
#!/bin/bash # Let's verify the designConfig propagation using different approaches # Check for designConfig usage in all files echo "Checking designConfig usage in component and TypeScript files..." rg "designConfig" -A 2 -B 2 # Check the initState function implementation to verify designConfig handling echo "Checking initState implementation..." ast-grep --pattern 'function initState($_) { $$$ }' # Look for any tagTypeWhenMultiple references echo "Checking tagTypeWhenMultiple usage..." rg "tagTypeWhenMultiple" -A 2 -B 2Length of output: 78994
shenjunjian
changed the title
| await page.getByRole('textbox', { name: '请选择' }).nth(0).click() | ||
| await page.waitForTimeout(100) |
There was a problem hiding this comment.
Remove duplicate click action to prevent test flakiness.
The duplicate click on the same textbox is unnecessary and could lead to flaky tests:
- The first click (line 8) already opens the cascader dropdown
- The second click (line 10) might immediately close it
- The timeout seems like a workaround for potential race conditions
Additionally, this change doesn't appear to be related to the PR's objective of adding designConfig for tag's type in the cascader component.
Apply this diff to fix the issue:
await page.getByRole('textbox', { name: '请选择' }).nth(0).click()
await expect(page.getByText('指南', { exact: true })).toHaveAttribute('title', '指南')
- await page.getByRole('textbox', { name: '请选择' }).nth(0).click()
- await page.waitForTimeout(100)📝 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.
| await page.getByRole('textbox', { name: '请选择' }).nth(0).click() | |
| await page.waitForTimeout(100) | |
| await page.getByRole('textbox', { name: '请选择' }).nth(0).click() | |
| await expect(page.getByText('指南', { exact: true })).toHaveAttribute('title', '指南') |
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
Cascadercomponent to the design library.tagTypeWhenMultiplefor dynamic tag coloring in multi-select scenarios.Improvements
Cascadercomponent by allowing design-related configurations.Bug Fixes
tiny-tagcomponent to improve dynamic behavior.Cascadercomponent to ensure proper interaction with the UI.