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.

Sign up for GitHub

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(drawer): [drawer] modify icon and demo problem #2460

Merged
merged 1 commit into from
Oct 29, 2024
Merged

fix(drawer): [drawer] modify icon and demo problem #2460

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
19 changes: 15 additions & 4 deletions examples/sites/demos/pc/app/drawer/use-through-method-composition-api.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@

<script lang="tsx" setup>
import { ref } from 'vue'
import { TinyDrawer, TinyButton } from '@opentiny/vue'
import { TinyDrawer, TinyButton, Notify } from '@opentiny/vue'
import { iconHelp } from '@opentiny/vue-icon'

let drawerInstance = null
Expand All @@ -24,15 +24,26 @@ const config = ref({
showFooter: true,
// 通过 events 监听事件
events: {
open: (instance) => console.log('open 事件', instance),
close: () => console.log('close 事件')
open: (instance) =>
Notify({
type: 'info',
title: 'open 事件',
message: `${instance.title}`,
position: 'top-right'
}),
close: () =>
Notify({
type: 'info',
title: 'close 事件',
position: 'top-right'
})
Comment on lines +27 to +39
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 differentiating notification types and enhancing message consistency

While the notifications are an improvement over console logs, consider these enhancements:

  1. Use different notification types for different events (e.g., 'success' for open, 'warning' for close)
  2. Add a message for the close event for consistency with the open event
    open: (instance) =>
      Notify({
-       type: 'info',
+       type: 'success',
        title: 'open 事件',
        message: `${instance.title}`,
        position: 'top-right'
      }),
    close: () =>
      Notify({
-       type: 'info',
+       type: 'warning',
        title: 'close 事件',
+       message: '抽屉已关闭',
        position: 'top-right'
      })
📝 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
open: (instance) =>
Notify({
type: 'info',
title: 'open 事件',
message: `${instance.title}`,
position: 'top-right'
}),
close: () =>
Notify({
type: 'info',
title: 'close 事件',
position: 'top-right'
})
open: (instance) =>
Notify({
type: 'success',
title: 'open 事件',
message: `${instance.title}`,
position: 'top-right'
}),
close: () =>
Notify({
type: 'warning',
title: 'close 事件',
message: '抽屉已关闭',
position: 'top-right'
})

💡 Codebase verification

Tests and documentation updates needed for notification behavior

The changes require:

  • No test files exist for the drawer component
  • Documentation lacks information about notification events
  • Notification behavior is only present in demo files but not documented

Action items:

  • Add unit tests covering the notification behavior on open/close events
  • Update documentation in examples/sites/demos/pc/app/drawer/webdoc/drawer.en.md to include notification event details
  • Consider adding a dedicated events section in documentation showcasing notification feedback
🔗 Analysis chain

Verify test coverage and documentation updates

The changes introduce new behavior with notifications and UI modifications. Please ensure:

  1. Add tests for the notification behavior
  2. Update component documentation to reflect the new notification feedback
  3. Add examples demonstrating the notification behavior in the component's demo

Also applies to: 46-46

Let me check the content of the documentation files to verify if they cover the notification behavior.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check for existing tests and documentation
echo "Checking for existing test files..."
fd -e spec.js -e spec.ts -e test.js -e test.ts drawer

echo "Checking for documentation files..."
fd -e md drawer

Length of output: 475

Script:

#!/bin/bash
# Check documentation content for notification-related information
echo "Checking PC documentation content..."
cat examples/sites/demos/pc/app/drawer/webdoc/drawer.en.md

echo -e "\nChecking demo files for notification examples..."
rg -l "Notify" "examples/sites/demos/pc/app/drawer/"

Length of output: 546

},
// 通过属性 customSlots 设置插槽, 插槽内容可以是 string | VNode | ({h, $drawer}) => VNode
customSlots: {
// 使用 h 函数
default: (h) => h('p', { class: '' }, '抽屉主体内容。'),
// 返回 VNode 节点的方法, 可通过参数中 $drawer 访问到组件实例
headerRight: () => <IconHelp></IconHelp>,
headerRight: () => <IconHelp style="width:20px;height:20px"></IconHelp>,
// 直接赋值 VNode
footer: (
<TinyButton type="primary" onClick={closeDrawer}>
Expand Down
29 changes: 20 additions & 9 deletions examples/sites/demos/pc/app/drawer/use-through-method.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
</template>

<script lang="tsx">
import { TinyDrawer, TinyButton } from '@opentiny/vue'
import { TinyDrawer, TinyButton, Notify } from '@opentiny/vue'
import { iconHelp } from '@opentiny/vue-icon'

export default {
Expand All @@ -18,6 +18,9 @@ export default {
}
},
methods: {
closeDrawer() {
this.drawerInstance.close()
},
showDrawer() {
const IconHelp = iconHelp()
const config = {
Expand All @@ -27,27 +30,35 @@ export default {
showFooter: true,
// 通过 events 监听事件
events: {
open: (instance) => console.log('open 事件', instance),
close: () => console.log('close 事件')
open: (instance) =>
Notify({
type: 'info',
title: 'open 事件',
message: `${instance.title}`,
position: 'top-right'
}),
close: () =>
Notify({
type: 'info',
title: 'close 事件',
position: 'top-right'
})
Comment on lines +33 to +45
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 internationalizing notification messages

The notification implementation provides better user feedback compared to console logs. However, consider extracting the message strings for internationalization support.

 events: {
   open: (instance) =>
     Notify({
       type: 'info',
-      title: 'open 事件',
+      title: this.$t('drawer.events.open'),
       message: `${instance.title}`,
       position: 'top-right'
     }),
   close: () =>
     Notify({
       type: 'info',
-      title: 'close 事件',
+      title: this.$t('drawer.events.close'),
       position: 'top-right'
     })
 }

Committable suggestion was skipped due to low confidence.

},
// 通过属性 customSlots 设置插槽, 插槽内容可以是 string | VNode | ({h, $drawer}) => VNode
customSlots: {
// 使用 h 函数
default: (h) => h('p', { class: '' }, '抽屉主体内容。'),
// 返回 VNode 节点的方法, 可通过参数中 $drawer 访问到组件实例
headerRight: () => <IconHelp></IconHelp>,
headerRight: () => <IconHelp style="width:20px;height:20px"></IconHelp>,
// 直接赋值 VNode
footer: (
<Button type="primary" onClick={this.closeDrawer}>
<TinyButton type="primary" onClick={this.closeDrawer}>
点击关闭
</Button>
</TinyButton>
)
}
}
this.drawerInstance = TinyDrawer.service(config)
},
closeDrawer() {
this.drawerInstance.close()
}
}
}
Expand Down
Loading