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

refactor(theme): [filter-panel] refactor filter-panel theme vars #2288

Merged
merged 2 commits into from
Oct 18, 2024
Merged

refactor(theme): [filter-panel] refactor filter-panel theme vars #2288

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f97dacb
refactor(theme): [filter-panel] refactor filter-panel theme vars
zzcr Oct 17, 2024
0f06b09
refactor(theme): [filter-panel] refactor filter-panel theme vars
zzcr Oct 17, 2024
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/filter-panel/basic-usage.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ test('基本用法', async ({ page }) => {

await button.click()
await popPanel.isVisible()
await page.getByRole('radio', { name: '等于' }).click()
await page.locator('label').filter({ hasText: '等于' }).click()
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 the trade-offs of the new selector approach

The change from page.getByRole('radio', { name: '等于' }).click() to page.locator('label').filter({ hasText: '等于' }).click() has both advantages and potential drawbacks:

Pros:

  • More flexible if the element structure changes
  • Might work better with custom-styled radio buttons

Cons:

  • Less accessibility-focused
  • Potentially less specific if there are multiple elements with the same text

While the new approach works, it's generally recommended to use role-based selectors when possible for better accessibility testing. Consider using a data attribute for more specific targeting:

page.locator('[data-testid="equal-radio"]').click()

This approach combines specificity with maintainability. If you decide to keep the current implementation, please ensure it's consistently applied across all similar scenarios in the test suite.

await button.getByText('物品数量等于').isVisible()
await inputEl.click()
await inputEl.fill('10')
Expand Down
2 changes: 1 addition & 1 deletion examples/sites/demos/pc/app/filter-panel/event.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ test('事件', async ({ page }) => {
// handle-clear事件
await button.click()
await popPanel.isVisible()
await page.getByRole('radio', { name: '等于' }).click()
await page.locator('label').filter({ hasText: '等于' }).click()
await expect(button).toHaveText('物品数量等于')
await clear.click()
await expect(button).toHaveText('物品数量')
Expand Down
84 changes: 41 additions & 43 deletions packages/theme/src/filter-box/index.less
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,119 +16,117 @@
@filter-box-prefix-cls: ~'@{css-prefix}filter-box';

.@{filter-box-prefix-cls} {
.component-css-vars-filter-box();
.inject-FilterBox-vars();

width: fit-content;
height: var(--ti-filter-box-btn-height);
line-height: var(--ti-filter-box-btn-height);
height: var(--tv-FilterBox-btn-height);
line-height: var(--tv-FilterBox-btn-height);
border-radius: 4px;
background: var(--ti-filter-box-btn-bg-color);
color: #161e26;
padding: 4px var(--ti-filter-box-btn-padding-horizontal);
background: var(--tv-FilterBox-btn-bg-color);
color: var(--tv-FilterBox-btn-text-color);
padding: 4px var(--tv-FilterBox-btn-padding-x);
Comment on lines +22 to +27
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

Issues Found: Missing CSS Variable Definitions

The updated CSS variables in index.less (--tv-FilterBox-btn-height, --tv-FilterBox-btn-bg-color, --tv-FilterBox-btn-text-color, --tv-FilterBox-btn-padding-x) are not defined in vars.less. This may lead to styling issues in the FilterBox component.

  • Ensure that all updated variables are properly defined in packages/theme/src/filter-box/vars.less.
🔗 Analysis chain

LGTM: CSS variable naming convention update

The update of CSS variable names from --ti- prefix to --tv- prefix is consistent with the refactoring effort. The new names are more specific to the FilterBox component, improving clarity and maintainability.

Please ensure that these new variables are properly defined in the corresponding vars.less file. Run the following script to verify:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the new CSS variable definitions in vars.less

# Test: Search for the new variable definitions
rg --type less -e '--tv-FilterBox-btn-height' -e '--tv-FilterBox-btn-bg-color' -e '--tv-FilterBox-btn-text-color' -e '--tv-FilterBox-btn-padding-x' -e '--tv-FilterBox-btn-font-size' packages/theme/src/filter-box/vars.less

Length of output: 689

display: flex;
align-items: center;
font-size: 12px;

&:hover {
background: var(--ti-filter-box-btn-hover-bg-color);
}
font-size: var(--tv-FilterBox-btn-font-size);

&.disabled {
cursor: not-allowed;

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 disabled state colors with variables

The .disabled class and .filter-icon-close use hardcoded color values (e.g., color: #b5bbc1;, fill: #b5bbc1;). Replacing these with theme variables improves maintainability and theming flexibility.

Example changes:

- color: #b5bbc1;
+ color: var(--tv-FilterBox-disabled-text-color);

- fill: #b5bbc1;
+ fill: var(--tv-FilterBox-disabled-icon-color);

Define these new variables in vars.less to correspond with the desired disabled state colors.

Also applies to: 109-109

.title label {
color: #b5bbc1;
color: var(--tv-FilterBox-btn-disabled-text-color);
}

.value {
color: #5c6976;
color: var(--tv-FilterBox-btn-disabled-text-color);
}

svg.filter-box-icon {
fill: #b5bbce;
fill: var(--tv-FilterBox-icon-color-disabled);
cursor: not-allowed;
}
}

.title {
height: inherit;
line-height: var(--ti-filter-box-btn-title-line-height);
font-size: var(--ti-filter-box-btn-font-size);
/* TODO: title 这个类名不符合组件类名规范，被公共样式污染，故在此处重置间距为 0.计划优化这个类名 */
margin-right: 0px;
line-height: var(--tv-FilterBox-btn-title-line-height);
font-size: var(--tv-FilterBox-btn-font-size);
margin-right: 0;

label {
display: inline-block;
max-width: 100px;
overflow: hidden;
text-overflow: ellipsis;
white-space: nowrap;
color: var(--ti-filter-box-btn-text-color);
color: var(--tv-FilterBox-btn-text-color);
margin-right: 8px;
margin-top: 3px;
}

&.active label {
color: var(--ti-filter-box-btn-hover-text-color);
color: var(--tv-FilterBox-btn-active-text-color);
}

// 问号图标
svg {
width: 16px;
height: 16px;
margin-right: var(--ti-filter-box-help-btn-margin-right);
fill: var(--ti-filter-box-help-btn-icon-color);
width: var(--tv-FilterBox-icon-size);
height: var(--tv-FilterBox-icon-size);
margin-right: var(--tv-FilterBox-help-btn-margin-right);
fill: var(--tv-FilterBox-icon-color);
position: relative;
top: var(--ti-filter-box-help-btn-position-top);
top: var(--tv-FilterBox-help-btn-position-top);
vertical-align: super;

&:hover {
fill: var(--ti-filter-box-btn-hover-text-color);
fill: var(--tv-FilterBox-icon-color-hover);
}
}
}

.value {
max-width: 100px;
height: inherit;
line-height: 1.5;
margin-right: 4px;
vertical-align: middle;
overflow: hidden;
text-overflow: ellipsis;
white-space: nowrap;
font-size: 12px;
color: #161e26;
font-size: var(--tv-FilterBox-btn-font-size);
color: var(--tv-color-text);

&.placeholder {
color: #aeaeae;
color: var(--tv-FilterBox-btn-text-color-placeholder);
}
}

.filter-box-icon {
width: var(--ti-filter-box-expand-btn-icon-size);
height: var(--ti-filter-box-expand-btn-icon-size);
width: var(--tv-FilterBox-icon-size);
height: var(--tv-FilterBox-icon-size);
cursor: pointer;
fill: var(--ti-filter-box-expand-btn-icon-color);
fill: var(--tv-FilterBox-icon-color);
transition: transform 0.3s;

&.is-reverse {
transform: rotate(180deg);
}
}

&:hover {
.filter-box-icon {
fill: var(--ti-filter-box-expand-btn-icon-color-hover);
}
}

.filter-icon-close {
width: 16px;
height: 16px;
width: var(--tv-FilterBox-icon-size);
height: var(--tv-FilterBox-icon-size);
cursor: pointer;
fill: #b5bbc1;
fill: var(--tv-FilterBox-icon-color);

&:hover {
fill: #78828d;
fill: var(--tv-FilterBox-icon-color-hover);
}
}

&:not(.disabled):hover {
background: var(--tv-FilterBox-btn-hover-bg-color);

.filter-box-icon {
fill: var(--tv-FilterBox-icon-color-hover);
}
}

Expand Down
50 changes: 28 additions & 22 deletions packages/theme/src/filter-box/vars.less
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,33 +9,39 @@
* A PARTICULAR PURPOSE. SEE THE APPLICABLE LICENSES FOR MORE DETAILS.
*
*/
.component-css-vars-filter-box() {
.inject-FilterBox-vars() {
// 按钮高度
--ti-filter-box-btn-height: var(--ti-common-size-height-small, 24px);
--tv-FilterBox-btn-height: var(--tv-size-height-xs);
// 按钮背景色
--ti-filter-box-btn-bg-color: var(--ti-common-color-bg-6, #f5f5f5);
// 按钮展开图标尺寸
--ti-filter-box-btn-hover-bg-color: var(--ti-common-color-bg-6, #f5f5f5);
--tv-FilterBox-btn-bg-color: var(--tv-color-bg);
// 按钮悬浮背景色
--tv-FilterBox-btn-hover-bg-color: var(--tv-color-bg-hover-1);
// 按钮水平内边距
--ti-filter-box-btn-padding-horizontal: var(--ti-common-space-2x, 8px);
--tv-FilterBox-btn-padding-x: var(--tv-space-md);
// 按钮文本色
--ti-filter-box-btn-text-color: var(--ti-common-color-text-weaken, #808080);
// 按钮悬浮文本色和图标色
--ti-filter-box-btn-hover-text-color: var(--ti-common-color-text-primary, #191919);
--tv-FilterBox-btn-text-color: var(--tv-color-text-weaken);
// 按钮占位文本色
--tv-FilterBox-btn-text-color-placeholder: var(--tv-color-text-placeholder);
// 按钮激活文本色
--tv-FilterBox-btn-active-text-color: var(--tv-color-text-active);
// 按钮文本禁用色
--tv-FilterBox-btn-disabled-text-color: var(--tv-color-text-disabled);
// 按钮文本字号
--ti-filter-box-btn-font-size: var(--ti-common-font-size-0, 12px);
--tv-FilterBox-btn-font-size: var(--tv-font-size-sm);
// 按钮文本行高
--ti-filter-box-btn-title-line-height: var(--ti-common-line-height-number, 1.5);
// 按钮问号图标色
--ti-filter-box-help-btn-icon-color: var(--ti-common-color-text-primary, #191919);
--tv-FilterBox-btn-title-line-height: var(--tv-line-height-number);

// 按钮帮助图标右边距
--ti-filter-box-help-btn-margin-right: var(--ti-common-space-base, 4px);
// 按钮问号top定位(hide)
--ti-filter-box-help-btn-position-top: var(--ti-common-space-base, 4px);
// 按钮展开图标色
--ti-filter-box-expand-btn-icon-color: var(--ti-common-color-icon-normal, #808080);
// 按钮展开图标悬浮色
--ti-filter-box-expand-btn-icon-color-hover: var(--ti-common-color-icon-hover, #191919);
// 按钮展开图标尺寸
--ti-filter-box-expand-btn-icon-size: var(--ti-common-font-size-2, 16px);
--tv-FilterBox-help-btn-margin-right: var(--tv-space-sm);
// 按钮帮助图标上边距
--tv-FilterBox-help-btn-position-top: var(--tv-space-sm);

// 按钮图标色
--tv-FilterBox-icon-color: var(--tv-color-icon);
// 按钮图标悬浮色
--tv-FilterBox-icon-color-hover: var(--tv-color-icon-hover);
// 按钮图标禁用色
--tv-FilterBox-icon-color-disabled: var(--tv-color-icon-disabled);
// 按钮图标尺寸
--tv-FilterBox-icon-size: var(--tv-icon-size);
}
10 changes: 5 additions & 5 deletions packages/theme/src/filter-panel/index.less
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,21 +17,21 @@

.@{filter-panel-prefix-cls} {
&__popover {
.component-css-vars-filter-panel();
.inject-FilterPanel-vars();
}

&__popover.tiny-popover.tiny-popper {
min-width: 200px;
padding: var(--ti-filter-panel-padding-vertical) var(--ti-filter-panel-padding-horizontal);
padding: var(--tv-FilterPanel-padding-y) var(--tv-FilterPanel-padding-x);

&[x-placement^='bottom'] {
margin-top: var(--ti-filter-panel-margin-top);
box-shadow: var(--ti-filter-panel-bottom-box-shadow);
margin-top: var(--tv-FilterPanel-margin-top);
box-shadow: var(--tv-FilterPanel-bottom-box-shadow);
}

&[x-placement^='top'] {
margin-bottom: 4px;
box-shadow: var(--ti-filter-panel-top-box-shadow);
box-shadow: var(--tv-FilterPanel-top-box-shadow);
}
}
}
12 changes: 6 additions & 6 deletions packages/theme/src/filter-panel/vars.less
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,15 +9,15 @@
* A PARTICULAR PURPOSE. SEE THE APPLICABLE LICENSES FOR MORE DETAILS.
*
*/
.component-css-vars-filter-panel() {
.inject-FilterPanel-vars() {
// 面板上外边距
--ti-filter-panel-margin-top: var(--ti-common-space-2x, 8px);
--tv-FilterPanel-margin-top: var(--tv-space-md);
// 面板垂直内边距
--ti-filter-panel-padding-vertical: var(--ti-common-space-6x, 24px);
--tv-FilterPanel-padding-y: 24px;
// 面板水平内边距
--ti-filter-panel-padding-horizontal: var(--ti-common-space-6x, 24px);
--tv-FilterPanel-padding-x: 24px;
// 朝下的面板阴影
--ti-filter-panel-bottom-box-shadow: 0px 8px 24px 0px rgba(0, 0, 0, 0.18);
--tv-FilterPanel-bottom-box-shadow: var(--tv-shadow-4-down);
// 朝上的面板阴影
--ti-filter-panel-top-box-shadow: 8px 0px 24px 0px rgba(0, 0, 0, 0.18);
--tv-FilterPanel-top-box-shadow: var(--tv-shadow-4-up);
}
Loading