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

refactor(theme): [popover] refactor popover theme vars #2259

Merged
merged 1 commit into from
Oct 14, 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
84 changes: 41 additions & 43 deletions packages/theme/src/popover/index.less
Original file line number Diff line number Diff line change
Expand Up @@ -17,25 +17,23 @@
@popper-prefix-cls: ~'@{css-prefix}popper';

.@{popover-prefix-cls}.@{popper-prefix-cls} {
.component-css-vars-popover();
.inject-Popover-vars();

position: absolute;
background: var(--ti-popover-bg-color);
color: var(--ti-popover-text-color);
border-radius: var(--ti-popover-border-radius);
border: 1px solid var(--ti-popover-border-color);
padding: var(--ti-popover-padding-vertical) var(--ti-popover-padding-horizontal);
background: var(--tv-Popover-bg-color);
color: var(--tv-Popover-text-color);
border-radius: var(--tv-Popover-border-radius);
border: 1px solid transparent;
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 using theme variables for border properties for consistency

Currently, the border property is set to 1px solid transparent. To maintain consistency and enhance theming flexibility, consider using theme variables for the border width and color.

You could define variables like --tv-Popover-border-width and --tv-Popover-border-color and update the code as follows:

-border: 1px solid transparent;
+border: var(--tv-Popover-border-width) solid var(--tv-Popover-border-color);

Ensure these variables are defined in your theming variables file to allow for easier customization and maintenance.

📝 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
border: 1px solid transparent;
border: var(--tv-Popover-border-width) solid var(--tv-Popover-border-color);

padding: var(--tv-Popover-padding-y) var(--tv-Popover-padding-x);
z-index: 2000;
line-height: var(--ti-popover-line-height);
line-height: var(--tv-Popover-line-height);
text-align: justify;
font-size: var(--ti-popover-font-size);
font-family: var(--ti-popover-font-family);
font-weight: var(--ti-popover-font-weight);
box-shadow: var(--ti-popover-box-shadow);
font-size: var(--tv-Popover-font-size);
box-shadow: var(--tv-Popover-box-shadow);
word-break: break-all;

.popper__arrow,
.popper__arrow::after {
.popper__arrow:after {
position: absolute;
display: block;
width: 0;
Expand All @@ -46,89 +44,89 @@

.popper__arrow,
.@{popper-prefix-cls} .popper__arrow {
border-width: var(--ti-popover-arrow-border-width);
border-width: 6px;
-webkit-filter: drop-shadow(0 2px 12px rgba(0, 0, 0, 0.03));
filter: drop-shadow(0 2px 12px rgba(0, 0, 0, 0.03));
}

.popper__arrow::after {
.popper__arrow:after {
content: ' ';
border-width: var(--ti-popover-arrow-border-width);
border-width: 6px;
}

&[x-placement^='top'] {
margin-bottom: var(--ti-popover-placement-margin-vertical);
margin-bottom: var(--tv-Popover-placement-margin-y);

.popper__arrow {
bottom: calc(var(--ti-popover-arrow-height) * -1);
bottom: -8px;
left: 50%;
border-top-color: var(--ti-popover-arrow-border-color);
border-top-color: transparent;
border-bottom-width: 0;
border-top-width: var(--ti-popover-arrow-height);
border-top-width: 8px;

&::after {
&:after {
bottom: 1px;
margin-left: -6px;
border-top-color: var(--ti-popover-placement-arrow-after-border-color);
border-top-color: var(--tv-Popover-placement-arrow-after-border-color);
border-bottom-width: 0;
}
}
}

&[x-placement^='bottom'] {
margin-top: var(--ti-popover-placement-margin-vertical);
margin-top: var(--tv-Popover-placement-margin-y);

.popper__arrow {
top: calc(var(--ti-popover-arrow-height) * -1);
top: -8px;
left: 50%;
border-top-width: 0;
border-bottom-color: var(--ti-popover-arrow-border-color);
border-bottom-width: var(--ti-popover-arrow-height);
border-bottom-color: transparent;
border-bottom-width: 8px;

&::after {
&:after {
top: 1px;
margin-left: -6px;
border-top-width: 0;
border-bottom-color: var(--ti-popover-placement-arrow-after-border-color);
border-bottom-color: var(--tv-Popover-placement-arrow-after-border-color);
}
}
}

&[x-placement^='right'] {
margin-left: var(--ti-popover-placement-margin-horizontal);
margin-left: var(--tv-Popover-placement-margin-x);

.popper__arrow {
top: 50%;
left: calc(var(--ti-popover-arrow-height) * -1);
border-right-color: var(--ti-popover-arrow-border-color);
left: -8px;
border-right-color: transparent;
border-left-width: 0;
border-right-width: var(--ti-popover-arrow-height);
border-right-width: 8px;

&::after {
&:after {
bottom: -6px;
left: 1px;
border-right-color: var(--ti-popover-placement-arrow-after-border-color);
border-right-color: var(--tv-Popover-placement-arrow-after-border-color);
border-left-width: 0;
}
}
}

&[x-placement^='left'] {
margin-right: var(--ti-popover-placement-margin-horizontal);
margin-right: var(--tv-Popover-placement-margin-x);

.popper__arrow {
top: 50%;
right: calc(var(--ti-popover-arrow-height) * -1);
right: -8px;
border-right-width: 0;
border-left-color: var(--ti-popover-arrow-border-color);
border-left-width: var(--ti-popover-arrow-height);
border-left-color: transparent;
border-left-width: 8px;

&::after {
&:after {
right: 1px;
bottom: -6px;
margin-left: -6px;
border-right-width: 0;
border-left-color: var(--ti-popover-placement-arrow-after-border-color);
border-left-color: var(--tv-Popover-placement-arrow-after-border-color);
Comment on lines +47 to +129
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Replace hardcoded values with theme variables for better theming support

There are multiple instances of hardcoded pixel values in the arrow and placement-specific styles, such as border-width: 6px;, border-top-width: 8px;, margin-bottom: 8px;, etc. To improve consistency and enable easier theming customization, consider replacing these hardcoded values with theme variables.

For example, define theme variables like:

  • --tv-Popover-arrow-border-width
  • --tv-Popover-arrow-border-top-width
  • --tv-Popover-placement-margin
  • --tv-Popover-arrow-offset

Update your code accordingly. For instance:

-/* In general arrow styles */
-border-width: 6px;
+border-width: var(--tv-Popover-arrow-border-width);

-/* In placement-specific styles */
-margin-bottom: var(--tv-Popover-placement-margin-y);
+margin-bottom: var(--tv-Popover-placement-margin);

-bottom: -8px;
+bottom: calc(-1 * var(--tv-Popover-arrow-offset));

-border-top-width: 8px;
+border-top-width: var(--tv-Popover-arrow-border-top-width);

Ensure that these variables are defined in your variables file (vars.less) and set appropriate values. This change will enhance the flexibility of your component's styling and maintain consistent theming across the application.

Committable suggestion was skipped due to low confidence.

}
}
}
Expand All @@ -138,11 +136,11 @@
}

.@{popover-prefix-cls}__title {
color: var(--ti-popover-title-text-color);
font-size: var(--ti-popover-title-font-size);
font-weight: var(--ti-popover-title-weight);
color: var(--tv-Popover-title-text-color);
font-size: var(--tv-Popover-title-font-size);
font-weight: var(--tv-Popover-title-weight);
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 renaming --tv-Popover-title-weight to --tv-Popover-title-font-weight for consistency

To maintain consistent naming conventions across your theme variables, consider renaming --tv-Popover-title-weight to --tv-Popover-title-font-weight, matching the pattern used for other font-related variables like --tv-Popover-title-font-size.

line-height: 1;
margin-bottom: var(--ti-popover-title-padding-bottom);
margin-bottom: 8px;
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Replace hardcoded margin-bottom value with a theme variable

Currently, the margin-bottom for the popover title is set to 8px. To maintain consistency and theming flexibility, consider using a theme variable for this value.

For example:

-margin-bottom: 8px;
+margin-bottom: var(--tv-Popover-title-margin-bottom);

Ensure that --tv-Popover-title-margin-bottom is defined in your variables file with the desired value.

Committable suggestion was skipped due to low confidence.

}

&:focus,
Expand Down
46 changes: 16 additions & 30 deletions packages/theme/src/popover/vars.less
Original file line number Diff line number Diff line change
Expand Up @@ -10,47 +10,33 @@
*
*/

.component-css-vars-popover() {
// 弹框字体
--ti-popover-font-family: var(--ti-common-font-family, 'Helvetica', 'Arial', 'PingFangSC-Regular', 'Hiragino Sans GB', 'Microsoft YaHei', '微软雅黑', 'Microsoft JhengHei');
// 弹框字重
--ti-popover-font-weight: 400;
.inject-Popover-vars() {
// 弹框背景色
--ti-popover-bg-color: var(--ti-common-color-light, #ffffff);
--tv-Popover-bg-color: var(--tv-color-bg-secondary);
// 弹框内容文本色
--ti-popover-text-color: var(--ti-common-color-text-secondary, #595959);
// 弹框边框色
--ti-popover-border-color: transparent;
--tv-Popover-text-color: var(--tv-color-text-secondary);
// 弹框圆角
--ti-popover-border-radius: var(--ti-common-border-radius-4, 8px);
--tv-Popover-border-radius: var(--tv-border-radius-lg);
// 弹框内容字号
--ti-popover-font-size: 14px;
--tv-Popover-font-size: var(--tv-font-size-md);
// 弹框内容行高
--ti-popover-line-height: 20px;
--tv-Popover-line-height: var(--tv-line-height-number);
// 弹框标题文本色
--ti-popover-title-text-color: var(--ti-common-color-text-primary, #191919);
--tv-Popover-title-text-color: var(--tv-color-text);
// 弹框标题字号
--ti-popover-title-font-size: var(--ti-common-font-size-2, 16px);
--tv-Popover-title-font-size: var(--tv-font-size-lg);
// 弹框标题字重
--ti-popover-title-weight: 600;
// 弹框标题字重
--ti-popover-title-padding-bottom: 8px;
// 弹出框小箭头边框厚度
--ti-popover-arrow-border-width: calc(var(--ti-common-border-weight-2, 3px) * 2);
// 弹出框小箭头高度
--ti-popover-arrow-height: var(--ti-common-space-2x, 8px);
--tv-Popover-title-weight: var(--tv-font-weight-bold);
// 弹出框垂直外边距
--ti-popover-placement-margin-vertical: var(--ti-common-space-3x, 12px);
// 弹出框小箭头水平外边距(hide)
--ti-popover-placement-margin-horizontal: var(--ti-common-space-3x, 12px);
--tv-Popover-placement-margin-y: var(--tv-space-lg);
// 弹出框小箭头水平外边距
--tv-Popover-placement-margin-x: var(--tv-space-lg);
// 弹框小箭头填充色
--ti-popover-placement-arrow-after-border-color: var(--ti-common-color-light, #ffffff);
--tv-Popover-placement-arrow-after-border-color: var(--tv-color-bg-secondary);
// 弹框阴影
--ti-popover-box-shadow: 0px 2px 12px 0px rgba(0,0,0,0.08);
// 弹框小箭头边框色
--ti-popover-arrow-border-color: transparent;
--tv-Popover-box-shadow: var(--tv-shadow-2-down);
// 弹框垂直内边距
--ti-popover-padding-vertical: var(--ti-common-space-3x, 12px);
--tv-Popover-padding-y: var(--tv-space-lg);
// 弹框水平内边距
--ti-popover-padding-horizontal: var(--ti-common-space-4x, 16px);
--tv-Popover-padding-x: var(--tv-space-xl);
}
Loading