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

fix(dropdown): [dropdown] fixed the data disorder issue caused by the composite scenario of two-layer components and self-invoking groups. #2553

Merged
merged 4 commits into from
Nov 25, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion examples/sites/demos/pc/app/dropdown/visible-arrow.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,5 +17,5 @@ test('显示箭头', async ({ page }) => {
await dropdownTrigger.hover()
await expect(dropDownMenu).toBeVisible()
await expect(arrow).toBeVisible()
await expect(arrow).toHaveCSS('top', '-8px')
await expect(arrow).toHaveCSS('top', '-7px')
})
8 changes: 6 additions & 2 deletions packages/vue/src/dropdown-item/src/pc.vue
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,9 @@
:key="index"
:label="item[state.textField]"
:item-data="item"
:_constants="_constants"
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Define the _constants prop in props array.

The _constants prop is being used in the template but is not defined in the component's props array. This could lead to Vue warnings and potential runtime issues.

Add the prop definition:

props: [
  ...props,
+ '_constants',
  'disabled',
  // ... other props
],

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

:icon="item.icon"
:is-mono="true"
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider binding to parent's isMono prop instead of hardcoding.

The hardcoded true value for :is-mono will force all nested items into mono mode, regardless of the parent's setting. Consider binding to the parent's prop value:

-:is-mono="true"
+:is-mono="isMono"
📝 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
:is-mono="true"
:is-mono="isMono"

:disabled="item.disabled"
:divided="item.divided"
:tip="item.tip"
Expand Down Expand Up @@ -102,13 +104,15 @@ export default defineComponent({
'textField',
'tip',
'tipPosition',
'effect'
'effect',
'isMono'
Comment on lines +107 to +108
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add proper prop definitions with types and documentation.

The props should be properly defined with types, defaults, and documentation to improve maintainability and developer experience.

-'effect',
-'isMono'
+'effect': {
+  type: String,
+  default: 'dark',
+  validator: (value: string) => ['dark', 'light'].includes(value),
+  description: 'Controls the tooltip theme effect'
+},
+'isMono': {
+  type: Boolean,
+  default: false,
+  description: 'Controls the mono mode behavior for nested dropdown items'
+}

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

],
components: {
IconLeftWardArrow: iconLeftWardArrow()
},
setup(props, context) {
return setup({ props, context, renderless, api }) as unknown as IDropdownItemApi
// 修复 <双层组件 + 自调用组件> 复合场景导致的数据混乱问题
return setup({ props, context, renderless, api, mono: props.isMono }) as unknown as IDropdownItemApi
}
})
</script>
Loading