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(alert): [alert] Adapting to the SMB theme #2169

Merged
merged 1 commit into from
Sep 24, 2024
Merged

feat(alert): [alert] Adapting to the SMB theme #2169

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
16 changes: 8 additions & 8 deletions examples/sites/demos/pc/app/alert/basic-usage.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,14 @@ test('基本用法', async ({ page }) => {
const alertBox = demo.locator('.tiny-alert')

// 颜色边框正确
await expect(alertBox.first()).toHaveCSS('background-color', 'rgb(235, 246, 255)')
await expect(alertBox.first()).toHaveCSS('border-bottom-color', 'rgb(190, 204, 250)')
await expect(alertBox.nth(1)).toHaveCSS('background-color', 'rgb(255, 238, 237)')
await expect(alertBox.nth(1)).toHaveCSS('border-bottom-color', 'rgb(255, 188, 186)')
await expect(alertBox.nth(2)).toHaveCSS('background-color', 'rgb(237, 255, 249)')
await expect(alertBox.nth(2)).toHaveCSS('border-bottom-color', 'rgb(172, 242, 220)')
await expect(alertBox.nth(3)).toHaveCSS('background-color', 'rgb(255, 243, 232)')
await expect(alertBox.nth(3)).toHaveCSS('border-bottom-color', 'rgb(255, 208, 166)')
await expect(alertBox.first()).toHaveCSS('background-color', 'rgb(222, 236, 255)')
await expect(alertBox.first()).toHaveCSS('border-bottom-color', 'rgb(222, 236, 255)')
await expect(alertBox.nth(1)).toHaveCSS('background-color', 'rgb(252, 227, 225)')
await expect(alertBox.nth(1)).toHaveCSS('border-bottom-color', 'rgb(252, 227, 225)')
await expect(alertBox.nth(2)).toHaveCSS('background-color', 'rgb(230, 242, 213)')
await expect(alertBox.nth(2)).toHaveCSS('border-bottom-color', 'rgb(230, 242, 213)')
await expect(alertBox.nth(3)).toHaveCSS('background-color', 'rgb(255, 235, 209)')
await expect(alertBox.nth(3)).toHaveCSS('border-bottom-color', 'rgb(255, 235, 209)')

// 警告可见
const alertSuccess = demo.locator('.tiny-alert--success')
Expand Down
2 changes: 1 addition & 1 deletion examples/sites/demos/pc/app/alert/size.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,5 +6,5 @@ test('尺寸', async ({ page }) => {

const largeAlert = page.locator('.tiny-alert').nth(1)
await expect(largeAlert).toHaveClass(/tiny-alert--large/)
await expect(largeAlert).toHaveCSS('height', '68px')
await expect(largeAlert).toHaveCSS('height', '77px')
})
9 changes: 3 additions & 6 deletions packages/theme/src/alert/index.less
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
margin: var(--ti-alert-normal-margin-vertical) var(--ti-alert-normal-margin-horizontal);
display: flex;
justify-content: flex-start;
align-items: center;

.@{alert-prefix-cls}__content {
padding: var(--ti-alert-normal-content-padding-vertical) var(--ti-alert-normal-content-padding-horizontal);
Expand Down Expand Up @@ -64,7 +65,6 @@

.@{alert-prefix-cls}__icon {
font-size: var(--ti-alert-large-icon-font-size);
margin-top: var(--ti-alert-large-icon-margin-top);
vertical-align: top;
flex-shrink: 0;
}
Expand Down Expand Up @@ -146,13 +146,10 @@
&__icon {
font-size: var(--ti-alert-icon-font-size);
}
&__icon:first-child {
margin-top: var(--ti-alert-icon-margin-top);
}

&.is-center {
justify-content: center;
text-align: center;
align-items: center;
}

.is-custom {
Expand Down Expand Up @@ -192,7 +189,7 @@
font-size: var(--ti-alert-title-font-size);
color: var(--ti-alert-title-text-color);
font-weight: bold;
margin-top: var(--ti-alert-title-margin-top);
line-height: 20px;
}

& &__description {
Expand Down
10 changes: 2 additions & 8 deletions packages/theme/src/alert/vars.less
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,8 +48,6 @@
--ti-alert-simple-title-text-color: var(--ti-common-color-text-primary, #252b3a);
// 标题字号
--ti-alert-title-font-size: var(--ti-common-font-size-2);
// 标题图标顶部外边距
--ti-alert-icon-margin-top: calc(var(--ti-common-space-6) / 2);
// 标题图标字号
--ti-alert-icon-font-size: var(--ti-common-font-size-2, 16px);
// 描述内容行高
Expand Down Expand Up @@ -89,9 +87,7 @@
// large尺寸操作字号
--ti-alert-opration-font-size: var(--ti-common-font-size-base, 12px);
// large尺寸图标字号
--ti-alert-large-icon-font-size: var(--ti-common-font-size-5, 24px);
// large尺寸图标顶部外边距
--ti-alert-large-icon-margin-top: var(--ti-common-space-0);
--ti-alert-large-icon-font-size: var(--ti-common-font-size-4, 24px);
// large尺寸描述内容左内边距
--ti-alert-large-content-padding-left: var(--ti-common-space-2x);
// 大尺寸关闭图标字号
Expand Down Expand Up @@ -150,7 +146,7 @@
// simple类型边框色
--ti-alert-simple-border-color: var(--ti-common-color-info-border, #d3d4d6);
// simple类型图标色
--ti-alert-simple-icon-color: var(--ti-common-color-text-secondary, #575d6c);
--ti-alert-simple-icon-color: #808080;
Copy link

Choose a reason for hiding this comment

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

Consider using a theme variable instead of a hard-coded color.

The change from var(--ti-common-color-text-secondary, #575d6c) to #808080 for the simple icon color reduces theme flexibility. While this may align with the SMB theme requirements, it could lead to inconsistencies if the theme is updated in the future.

Consider one of the following alternatives:

  1. Use an existing theme variable that matches the desired color.
  2. Create a new theme variable for this specific use case.

For example:

--ti-alert-simple-icon-color: var(--ti-theme-simple-icon-color, #808080);

This approach maintains flexibility while still achieving the desired color for the SMB theme.

// simple类型链接色(hide)
--ti-alert-simple-link-text-color: var(--ti-common-color-primary-active, #7693f5);
// simple类型背景色
Expand All @@ -159,6 +155,4 @@
--ti-alert-simple-close-icon-color: var(--ti-alert-simple-icon-color);
// simple类型悬浮关闭图标色
--ti-alert-simple-close-hover-icon-color: var(--ti-alert-simple-icon-color);
// 标题的上边距
--ti-alert-title-margin-top: var(--ti-common-size-base);
}
3 changes: 2 additions & 1 deletion packages/vue/src/alert/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,8 @@ export const $constants = {
success: 'icon-success',
error: 'icon-error',
info: 'icon-help',
warning: 'icon-warning'
warning: 'icon-warning',
simple: 'icon-help'
Comment on lines +20 to +21
Copy link

Choose a reason for hiding this comment

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

Tip

Codebase Verification

Incomplete implementation of the new 'simple' alert type

The addition of the 'simple' alert type in the ICON_MAP is not fully supported across the codebase. The TITLE_MAP and type prop definitions do not include 'simple', and no other references to the 'simple' type were found. Please ensure that the 'simple' type is consistently implemented throughout the codebase to avoid potential inconsistencies and user confusion.

Analysis chain

Consider refining the implementation of the new 'simple' alert type

The addition of a 'simple' alert type aligns with the PR objective of adapting to the SMB theme. However, there are a few points to consider:

  1. The 'simple' type uses the same icon as 'info' (icon-help). This might lead to confusion for users. Consider using a distinct icon for 'simple' to differentiate it from 'info'.

  2. The TITLE_MAP object hasn't been updated to include a title for the 'simple' type. This might cause issues when trying to display a title for a 'simple' alert.

  3. The type prop definition in alertProps still defaults to 'info' and doesn't mention 'simple' as a possible value. This should be updated to include the new 'simple' type.

To address these points, consider the following changes:

  1. Use a distinct icon for 'simple':
 ICON_MAP: {
   success: 'icon-success',
   error: 'icon-error',
   info: 'icon-help',
   warning: 'icon-warning',
-  simple: 'icon-help'
+  simple: 'icon-simple' // Replace with an appropriate icon
 },
  1. Add a title for 'simple' in TITLE_MAP:
 TITLE_MAP: {
   success: 'ui.alert.success',
   error: 'ui.alert.error',
   info: 'ui.alert.info',
-  warning: 'ui.alert.warning'
+  warning: 'ui.alert.warning',
+  simple: 'ui.alert.simple'
 },
  1. Update the type prop definition:
 type: {
   type: String,
-  default: 'info'
+  default: 'info',
+  validator: (value: string) => ['success', 'error', 'info', 'warning', 'simple'].includes(value)
 },

To ensure full compatibility with the new 'simple' type, run the following script to check for any other occurrences of alert types that might need updating:

This will help identify any other locations where the alert types are used or checked, ensuring comprehensive implementation of the new 'simple' type.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Search for alert type references in the codebase

echo "Searching for alert type references:"
rg --type typescript --type vue "type.*(?:success|error|info|warning)" -g '!**/index.ts'

Length of output: 208

Script:

#!/bin/bash
# Description: Search for alert type references in the codebase

echo "Searching for alert type references:"
rg --type ts --type tsx --type vue "type.*(?:success|error|info|warning)" -g '!**/index.ts'

Length of output: 204

Script:

#!/bin/bash
# Description: Search for alert type references in the codebase

echo "Searching for alert type references:"
rg --type typescript --type typescriptreact --type vue "type.*(?:success|error|info|warning)" -g '!**/index.ts'

Length of output: 231

Script:

#!/bin/bash
# Description: Search for alert type references in the codebase using file extensions

echo "Searching for alert type references:"
rg "type.*(?:success|error|info|warning)" -g "*.ts" -g "*.tsx" -g "*.vue" -g "!**/index.ts"

Length of output: 70336

},
TITLE_MAP: {
success: 'ui.alert.success',
Expand Down
Loading