fix: stripe table style for element plus, fixed: #4501 #4502
Conversation
mynetfan
requested review from
anncwb,
vince292007 and
a team
as code owners
|
WalkthroughThis pull request introduces new features and enhancements in a Vue.js application, including a new data structure for a table component, additional CSS variables for design tokens, and updates to the design configuration. The changes involve the addition of a Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (3)
apps/web-ele/src/views/demos/element/index.vue (2)
42-49: Consider adding a type definition for tableData.The tableData constant is correctly defined with appropriate demo data. However, for better type safety and code maintainability, consider adding a type definition for the table row structure.
Here's a suggested improvement:
interface TableRow { prop1: string; prop2: string; } const tableData: TableRow[] = [ { prop1: '1', prop2: 'A' }, // ... rest of the data ];
86-91: Consider internationalizing table column labels.The ElTable is correctly implemented with the stripe attribute, which addresses the PR objective. The data binding and column definitions are also correct.
For better internationalization support, consider using translation keys for the column labels instead of hardcoded Chinese text. For example:
<ElTable.TableColumn :label="$t('table.column1')" prop="prop1" /> <ElTable.TableColumn :label="$t('table.column2')" prop="prop2" />Ensure to add corresponding translations in your i18n configuration.
packages/effects/hooks/src/use-design-tokens.ts (1)
260-260: LGTM! Consider adding a dark mode condition for consistency.The addition of the
--el-fill-color-lightervariable is appropriate and aligns with the PR objective of fixing the stripe table style for ElementPlus in dark mode.For consistency with other color variables, consider adding a dark mode condition similar to the surrounding code. Here's a suggested improvement:
- '--el-fill-color-lighter': getCssVariableValue('--accent-lighter'), + '--el-fill-color-lighter': isDark.value + ? getCssVariableValue('--accent-lighter') + : getCssVariableValue('--accent-50'),This change would make the variable behave consistently with other color variables in both light and dark modes.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- apps/web-ele/src/views/demos/element/index.vue (3 hunks)
- internal/tailwind-config/src/index.ts (1 hunks)
- packages/@core/base/design/src/design-tokens/dark/index.css (1 hunks)
- packages/@core/base/design/src/design-tokens/default/index.css (1 hunks)
- packages/effects/hooks/src/use-design-tokens.ts (1 hunks)
Additional comments not posted (3)
apps/web-ele/src/views/demos/element/index.vue (2)
10-10: LGTM: ElTable import added correctly.The ElTable component is properly imported from element-plus, which is necessary for its usage in the template.
Line range hint
1-93: Overall implementation looks good and addresses the PR objective.The changes successfully introduce a striped table using Element Plus, which aligns with the PR objective of fixing the stripe table style. The implementation is correct and functional.
A few minor suggestions were made to improve type safety and internationalization support, but these are not critical for the current PR objective.
To ensure that the stripe style is working correctly in dark mode as mentioned in the PR objectives, please run the following verification:
This script will help identify any potential conflicts or overrides that might affect the stripe style in dark mode.
packages/@core/base/design/src/design-tokens/dark/index.css (1)
56-56: Approved: Addition of--accent-lightervariable for improved table stylingThe introduction of the
--accent-lighterCSS variable with the value216 5% 11%is a good approach to address the issue with zebra stripe styling for tables in dark mode. This new variable provides a slightly lighter shade compared to the existing--accentvariable, which should create a subtle contrast for alternating table rows.This change aligns well with the PR objectives and is appropriately categorized as a bug fix and non-breaking change.
To ensure this change fully addresses the reported issue (#4501), please run the following verification:
This script will help verify if the new variable is being used correctly in table styles and if there are any remaining tasks related to this issue.
| @@ -29,6 +29,7 @@ const shadcnUiColors = { | |||
| DEFAULT: 'hsl(var(--accent))', | |||
| foreground: 'hsl(var(--accent-foreground))', | |||
| hover: 'hsl(var(--accent-hover))', | |||
| lighter: 'has(val(--accent-lighter))', | |||
There was a problem hiding this comment.
💡 Codebase verification
Potential Issue with lighter Property Syntax
The lighter property in the shadcnUiColors object uses the value 'has(val(--accent-lighter))', which is inconsistent with other color properties that typically use the hsl(var(--accent-*)) format. Additionally, the has() and val() functions are not standard CSS functions and may cause configuration issues.
Could you please clarify:
- Is this the intended syntax for defining the
lighteraccent color? - How does this change contribute to the PR's objective of fixing the stripe table style issue in dark mode?
Analysis chain
Clarify the lighter property value and its relation to the PR objective
The addition of the lighter property to the accent color is noted. However, there are a few concerns:
- The value
'has(val(--accent-lighter))'doesn't follow the pattern used for other color properties in this file, which typically use the formathsl(var(--accent-*)). - The
has()andval()functions are not standard CSS functions, which might cause issues in the Tailwind configuration. - It's unclear how this change relates to the PR's objective of fixing the stripe table style for ElementPlus in dark mode.
Could you please clarify:
- Is this the intended syntax for defining the
lighteraccent color? - How does this change contribute to fixing the stripe table style issue mentioned in the PR description?
To check if this syntax is used elsewhere in the project:
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar syntax in other configuration files
rg --type=typescript "has\(val\(" -g "*.ts"
Length of output: 80
Script:
#!/bin/bash
# Search for similar syntax in other TypeScript configuration files
rg --type ts "has\\(val\\(--accent-lighter\\)\\)" -g "*.ts"
Length of output: 138
| @@ -53,6 +53,7 @@ | |||
|
|
|||
| /* Used for accents such as hover effects on <DropdownMenuItem>, <SelectItem>...etc */ | |||
| --accent: 240 5% 96%; | |||
| --accent-lighter: 240 0% 98%; | |||
There was a problem hiding this comment.
💡 Codebase verification
Unused --accent-lighter Variable Detected
The --accent-lighter custom property has been added in both dark and default design tokens:
packages/@core/base/design/src/design-tokens/dark/index.css:--accent-lighter: 216 5% 11%;packages/@core/base/design/src/design-tokens/default/index.css:--accent-lighter: 240 0% 98%;
However, there are no references to --accent-lighter in other parts of the codebase.
Please clarify how this addition contributes to the PR's objective of fixing the stripe table style for ElementPlus in dark mode.
Analysis chain
New custom property added: Clarification needed
The addition of the --accent-lighter custom property is noted. This new variable introduces a very light shade (almost white) to the accent color palette.
However, it's not immediately clear how this addition relates to the PR objective of fixing the stripe table style for ElementPlus in dark mode. Could you please clarify:
- How does this new variable contribute to addressing the issue with table styles in dark mode?
- Are there other changes in different files that utilize this new variable?
Additionally, consider adding a comment to document the purpose and intended use of this new custom property. This will help maintain clarity for future developers working with these design tokens.
To better understand the usage of this new variable, let's check if it's being used in other files:
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of the new --accent-lighter variable
rg --type css "--accent-lighter"
Length of output: 451
Script:
#!/bin/bash
# Corrected search for usage of the new --accent-lighter variable in CSS files
rg --type css "--accent-lighter"
Length of output: 451
Script:
#!/bin/bash
# Corrected search for usage of the new --accent-lighter variable in CSS files
rg --type css -e "--accent-lighter"
Length of output: 222
|
有多余的合并commit，重新提交PR |
Description
修复ElementPlus组件库的表格斑马纹样式在暗黑模式下的问题
Type of change
Please delete options that are not relevant.
pnpm-lock.yamlunless you introduce a new test example.Summary by CodeRabbit
New Features
Bug Fixes
Documentation