Skip to content

fix(base-select): [base-select] fix style issues in the old theme #2535

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

Merged
merged 2 commits into from
Nov 20, 2024
Merged

fix(base-select): [base-select] fix style issues in the old theme #2535

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
1dd8136
fix(base-select): fix style issues in the old theme
kagol Nov 19, 2024
1f7e188
test(base-select): fix e2e test of size
kagol Nov 20, 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
3 changes: 2 additions & 1 deletion examples/sites/demos/pc/app/base-select/collapse-tags-composition-api.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,8 @@ const options = ref([
{ value: '选项2', label: '上海' },
{ value: '选项3', label: '天津' },
{ value: '选项4', label: '重庆超长超长超长超长超长超长超长超长超长' },
{ value: '选项5', label: '深圳' }
{ value: '选项5', label: '深圳' },
{ value: '选项6', label: '广州' }
])

const value1 = ref(['选项1', '选项2'])
Expand Down
2 changes: 1 addition & 1 deletion examples/sites/demos/pc/app/base-select/size.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,5 +54,5 @@ test('mini 尺寸', async ({ page }) => {

await expect(input).toHaveClass(/tiny-input-mini/)
await expect(tag.nth(0)).toHaveClass(/tiny-tag--mini tiny-tag--light/)
expect(height).toBeCloseTo(28, 1)
expect(height).toBeCloseTo(24, 1)
})
3 changes: 2 additions & 1 deletion examples/sites/demos/pc/app/base-select/slot-panel-composition-api.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,8 @@
return {
...node,
currentLabel: node.label,
value: node.id
value: node.id,
isTree: true
Comment on lines +43 to +44
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Inconsistent node structure across select scenarios

The isTree property is only added to nodes in the multi-select scenario (场景2), but not in the single select (场景1) or searchable select (场景3) scenarios. This inconsistency could lead to issues in components that consume this data.

Consider applying this change consistently across all scenarios:

// 场景1
updateSelectedData({
  ...data,
  currentLabel: data.label,
  value: data.id,
+ isTree: true,
  state: {
    currentLabel: data.label
  }
})

// 场景3
updateSelectedData({
  ...data,
  currentLabel: data.label,
  value: data.id,
+ isTree: true,
  state: {
    currentLabel: data.label
  }
})

Committable suggestion skipped: line range outside the PR's diff.

}
})
)
Expand Down
3 changes: 2 additions & 1 deletion examples/sites/demos/pc/app/base-select/slot-panel.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,8 @@
return {
...node,
currentLabel: node.label,
value: node.id
value: node.id,
isTree: true
Comment on lines +43 to +44
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 standardizing the node structure across all scenarios

While adding isTree: true helps identify tree-sourced selections, I notice this property is only added in the multi-select scenario but not in the single-select or searchable scenarios. This inconsistency could lead to issues when handling selected nodes elsewhere in the application.

Consider applying this change to all three scenarios for consistency:

// Single select scenario
updateSelectedData({
  ...data,
  currentLabel: data.label,
  value: data.id,
+ isTree: true,
  state: {
    currentLabel: data.label
  }
})

// Searchable scenario
updateSelectedData({
  ...data,
  currentLabel: data.label,
  value: data.id,
+ isTree: true,
  state: {
    currentLabel: data.label
  }
})

Committable suggestion skipped: line range outside the PR's diff.

}
})
)
Expand Down
2 changes: 1 addition & 1 deletion packages/renderless/src/base-select/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1602,7 +1602,7 @@ export const mounted =
state.completed = true

// tiny 新增： sizeMap适配不同主题
const defaultSizeMap = { default: 32, mini: 24, small: 36, medium: 40 }
const defaultSizeMap = { medium: 40, default: 32, small: 28, mini: 24 }
const sizeMap = designConfig?.state?.sizeMap || defaultSizeMap

if (props.multiple && Array.isArray(props.modelValue) && props.modelValue.length > 0) {
Expand Down
18 changes: 18 additions & 0 deletions packages/theme/src/base-select/index.less
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,6 +203,24 @@
}
}

& {
.@{tag-prefix-cls} {
height: 20px;
}
}

&--medium {
.@{tag-prefix-cls} {
height: 24px;
}
}

&--small, &--mini {
.@{tag-prefix-cls} {
height: 16px;
}
}
Comment on lines +218 to +222
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

Small and mini sizes should have different heights

The current implementation using the same height (16px) for both small and mini sizes in the base-select component is inconsistent with the design system. Throughout the codebase:

  • The base design system defines different heights:

    • Mini size: --tv-size-height-xs: 24px
    • Small size: --tv-size-height-sm: 28px

  • This pattern is consistently followed across multiple components including Select, Button, Form, Picker, etc.

The tag heights should be adjusted to maintain consistency with the design system.

🔗 Analysis chain

Verify if small and mini sizes should share the same height

The current implementation uses the same height (16px) for both small and mini sizes. Please verify if this is intentional, as typically these sizes would have different values.

Based on the search results, I can see that throughout the codebase, small and mini sizes consistently have different heights. Specifically in the base variables (packages/theme/src/base/vars.less), we can see:

--tv-size-height-xs: 24px; // mini 尺寸
--tv-size-height-sm: 28px; // small 尺寸

Let me search for the specific tag height definitions to verify if there's any intentional override.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for other size definitions in the codebase to verify consistency
rg --type=less "height.*(?:small|mini)" -A 2 -B 2

Length of output: 25919

Script:

#!/bin/bash
# Search for tag height definitions in the codebase
rg --type=less "tag.*height|--tv-.*tag.*height" -B 2 -A 2

Length of output: 2861


&.is-hover-expand,
&.is-click-expand {
vertical-align: top;
Expand Down
1 change: 0 additions & 1 deletion packages/theme/src/select-dropdown/index.less
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,6 @@
}

.@{input-prefix-cls}__prefix {
display: block;
left: 0;
font-size: 0;

Expand Down
Loading