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

feat(popconfirm): [popconfirm] Adapting to the SMB theme #2168

Merged
merged 1 commit into from
Sep 24, 2024
Merged

feat(popconfirm): [popconfirm] Adapting to the SMB theme #2168

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
14 changes: 8 additions & 6 deletions examples/sites/demos/pc/app/popconfirm/type-composition-api.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,42 +1,44 @@
<template>
<div>
<tiny-popconfirm :title="title" type="info">
<tiny-popconfirm :title="title" type="info" :message="message">
<template #reference>
<tiny-button>info</tiny-button>
</template>
</tiny-popconfirm>

<tiny-popconfirm :title="title" type="error">
<tiny-popconfirm :title="title" type="error" :message="message">
<template #reference>
<tiny-button>error</tiny-button>
</template>
</tiny-popconfirm>

<tiny-popconfirm :title="title" type="warning">
<tiny-popconfirm :title="title" type="warning" :message="message">
<template #reference>
<tiny-button>warning</tiny-button>
</template>
</tiny-popconfirm>

<tiny-popconfirm :title="title" type="success">
<tiny-popconfirm :title="title" type="success" :message="message">
<template #reference>
<tiny-button>success</tiny-button>
</template>
</tiny-popconfirm>

<tiny-popconfirm :title="title" :type="TinyIconDel">
<tiny-popconfirm :title="title" :type="TinyIconDel" :message="message">
<template #reference>
<tiny-button>自定义</tiny-button>
</template>
</tiny-popconfirm>
</div>
</template>

<script setup>
import { ref } from 'vue'
import { Popconfirm as TinyPopconfirm, Button as TinyButton } from '@opentiny/vue'
import { iconDel } from '@opentiny/vue-icon'

const title = ref('确定要删除该安全组规则吗?')
const title = ref('提示标题')
const message = ref('安全组规则是推荐的，确定要删除该安全组规则吗?')
const TinyIconDel = iconDel()
</script>

Expand Down
13 changes: 7 additions & 6 deletions examples/sites/demos/pc/app/popconfirm/type.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,30 +1,30 @@
<template>
<div>
<tiny-popconfirm :title="title" type="info">
<tiny-popconfirm :title="title" type="info" :message="message">
Copy link

Choose a reason for hiding this comment

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

LGTM! Consider i18n for scalability.

The addition of the :message prop enhances the popconfirm component by providing more detailed information to the user, which aligns well with the PR objective of adapting to the SMB theme.

Consider wrapping the title and message content in i18n translations for better internationalization support:

<tiny-popconfirm :title="$t('popconfirm.info.title')" type="info" :message="$t('popconfirm.info.message')">

<template #reference>
<tiny-button>info</tiny-button>
</template>
</tiny-popconfirm>

<tiny-popconfirm :title="title" type="error">
<tiny-popconfirm :title="title" type="error" :message="message">
<template #reference>
<tiny-button>error</tiny-button>
</template>
</tiny-popconfirm>

<tiny-popconfirm :title="title" type="warning">
<tiny-popconfirm :title="title" type="warning" :message="message">
<template #reference>
<tiny-button>warning</tiny-button>
</template>
</tiny-popconfirm>

<tiny-popconfirm :title="title" type="success">
<tiny-popconfirm :title="title" type="success" :message="message">
<template #reference>
<tiny-button>success</tiny-button>
</template>
</tiny-popconfirm>

<tiny-popconfirm :title="title" :type="TinyIconDel">
<tiny-popconfirm :title="title" :type="TinyIconDel" :message="message">
<template #reference>
<tiny-button>自定义</tiny-button>
</template>
Expand All @@ -43,7 +43,8 @@ export default {
},
data() {
return {
title: '确定要删除该安全组规则吗?',
title: '提示标题',
message: '安全组规则是推荐的，确定要删除该安全组规则吗?',
Comment on lines +46 to +47
Copy link

Choose a reason for hiding this comment

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

Consider internationalization and clarity improvements.

The changes to the data() function provide more flexibility with a generic title and a detailed message. However, there are a few points to consider:

  1. The use of Chinese characters might limit internationalization.
  2. The generic title '提示标题' (which translates to "Prompt Title") might be too vague.
  3. The message is specific to deleting a security group rule, which might not be applicable to all use cases demonstrated in this component.

Consider the following improvements:

  1. Implement i18n for both title and message:
data() {
  return {
    title: this.$t('popconfirm.title'),
    message: this.$t('popconfirm.message'),
    // ...
  }
}
  1. Make the message more generic or provide different messages for different popconfirm types:
data() {
  return {
    title: this.$t('popconfirm.title'),
    messages: {
      info: this.$t('popconfirm.info.message'),
      error: this.$t('popconfirm.error.message'),
      warning: this.$t('popconfirm.warning.message'),
      success: this.$t('popconfirm.success.message'),
      custom: this.$t('popconfirm.custom.message'),
    },
    // ...
  }
}

Then update the template to use the appropriate message:

<tiny-popconfirm :title="title" type="info" :message="messages.info">
  <!-- ... -->
</tiny-popconfirm>

This approach would provide more flexibility and clarity for different popconfirm types while supporting internationalization.

TinyIconDel: iconDel()
}
}
Expand Down
9 changes: 3 additions & 6 deletions packages/theme/src/popconfirm/index.less
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,12 +41,8 @@
&__header {
display: flex;
align-items: center;
line-height: var(--ti-popconfirm-popover-line-height);
margin-bottom: var(--ti-popconfirm-popover-margin-bottom);
}
.no-message {
font-weight: 400;
}

&__icon {
height: var(--ti-popconfirm-popover-icon-width);
Expand All @@ -56,14 +52,14 @@
}

&__title {
flex: 1 1 auto;
font-size: var(--ti-popconfirm-popover-title-font-size);
font-weight: var(--ti-popconfirm-popover-title-font-weight);
line-height: var(--ti-popconfirm-popover-title-line-height);
}

&__content {
color: var(--ti-popconfirm-popover-text-color);
line-height: var(--ti-popconfirm-popover-line-height);
line-height: var(--ti-popconfirm-popover-content-line-height);
}

&__footer {
Expand All @@ -73,6 +69,7 @@
}

&__confirm-button {
margin-left: 8px;
Copy link

Choose a reason for hiding this comment

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

LGTM! Consider using a CSS variable for consistency.

Adding margin to the confirm button improves the layout. However, for better consistency with the design system and easier theming, consider using a CSS variable instead of a fixed pixel value.

Consider updating the code as follows:

-    margin-left: 8px;
+    margin-left: var(--ti-popconfirm-confirm-button-margin-left, 8px);

Don't forget to add this new variable to your vars.less file or wherever you define your CSS variables.

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
margin-left: 8px;
margin-left: var(--ti-popconfirm-confirm-button-margin-left, 8px);

width: 72px;
}

Expand Down
12 changes: 8 additions & 4 deletions packages/theme/src/popconfirm/vars.less
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,18 +11,20 @@
*/

.component-css-vars-popconfirm() {
// 提示弹窗字体行高
--ti-popconfirm-popover-line-height: 1.5;
// 提示弹窗title字体行高
--ti-popconfirm-popover-title-line-height:var(--ti-common-line-height-2);
// 提示弹窗content字体行高
--ti-popconfirm-popover-content-line-height: var(--ti-common-line-height-1);
// 底部按钮flex排列方式
--ti-popconfirm-popover-footer-justify-content: right;
// 提示弹窗字体大小
--ti-popconfirm-popover-footer-font-size: var(--ti-common-font-size-base, 12px);
// 提示弹窗字体颜色
--ti-popconfirm-popover-footer-font-color: #191919;
// 提示弹窗容器垂直内边距
--ti-popconfirm-popover-container-padding-vertical: unset;
--ti-popconfirm-popover-container-padding-vertical: var(--ti-common-space-base);
// 提示弹窗图标大小
--ti-popconfirm-popover-icon-width: var(--ti-common-size-4x);
--ti-popconfirm-popover-icon-width: var(--ti-common-size-5x);
--ti-popconfirm-popover-icon-margin-bottom: 2px;
// 提示弹窗图标上侧外边距
--ti-popconfirm-popover-icon-margin-right: var(--ti-common-space-base);
Expand All @@ -32,4 +34,6 @@
--ti-popconfirm-popover-title-font-size: var(--ti-common-font-size-2);
--ti-popconfirm-popover-title-font-weight: var(--ti-common-font-weight-bold);
--ti-popconfirm-popover-text-color: #191919;
// 标题距离内容下边距
--ti-popconfirm-popover-margin-bottom: var(--ti-common-space-2x);
}
2 changes: 1 addition & 1 deletion packages/vue/src/popconfirm/src/pc.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
:class="['tiny-popconfirm-popover__icon', type ? `tiny-popconfirm-popover--${type}` : '']"
>
</component>
<div class="tiny-popconfirm-popover__title" :class="[message ? '' : 'no-message']">
<div class="tiny-popconfirm-popover__title">
{{ title }}
</div>
</div>
Expand Down
Loading