refactor(pager): [pager] refactor pager style #2162
Conversation
WalkthroughThe pull request involves a significant renaming and restructuring of CSS variables in the pager component's Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (1)
Additional comments not posted (9)
|
|
[e2e-test-warn] The title of the Pull request should look like "fix(vue-renderless): [action-menu, alert] fix xxx bug". Please make sure you've read our contributing guide |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Outside diff range and nitpick comments (3)
packages/theme/src/pager/index.less (1)
37-39: Consider using a CSS variable for the margin value.To maintain consistency and flexibility, consider defining a CSS variable for the margin value instead of using a hardcoded value.
- margin-left: 16px; + margin-left: var(--tv-Pager-group-margin-left);packages/theme/src/pager/vars.less (2)
79-79: Clarify variable naming for custom prev/next text colorThe variable
--tv-Pager-custom-prev-next-text-colordefined in line 79 may benefit from clearer naming to indicate its purpose more precisely.Consider renaming it to
--tv-Pager-prev-next-custom-text-coloror--tv-Pager-prev-next-text-color-customto better reflect that it pertains to the custom text color of the previous and next buttons.Apply this diff if you choose to rename:
---tv-Pager-custom-prev-next-text-color: var(--tv-color-text-link); +--tv-Pager-prev-next-text-color-custom: var(--tv-color-text-link);Ensure that you update all instances where this variable is used.
81-87: Group margin variables for better readabilityLines 81 to 87 define margin variables for the previous and next buttons. Grouping related variables and adding descriptive comments can enhance readability.
Consider organizing the variables as follows:
// Previous button margins --tv-Pager-prev-margin-left: var(--tv-space-xl); --tv-Pager-prev-margin-right: var(--tv-space-md); // Next button margins --tv-Pager-next-margin-left: var(--tv-space-md); --tv-Pager-next-margin-right: 0;This grouping, along with comments, clarifies the purpose of each variable.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- .vscode/settings.json (1 hunks)
- packages/theme/src/pager/index.less (12 hunks)
- packages/theme/src/pager/old-theme.js (1 hunks)
- packages/theme/src/pager/vars.less (1 hunks)
Additional comments not posted (6)
.vscode/settings.json (1)
84-91: LGTM!The new tag is a valid addition to the "better-comments" extension settings. It provides a visually distinct style for specific comments without affecting the project code.
packages/theme/src/pager/index.less (4)
28-29: LGTM!The changes to the padding and color properties are consistent with the new CSS variable naming convention.
35-36: LGTM!The change to the font size property is consistent with the new CSS variable naming convention.
48-49: LGTM!The changes to various CSS properties are consistent with the new CSS variable naming convention. The updates follow a clear pattern and maintain the overall styling of the pager component.
Also applies to: 52-52, 62-62, 67-68, 73-73, 77-84, 88-88, 92-92, 97-100, 103-103, 111-111, 116-116, 122-125
146-156: LGTM!The changes to various CSS properties throughout the file are consistent with the new CSS variable naming convention. The updates maintain the existing styling while adhering to the new variable names.
Also applies to: 163-163, 170-177, 183-186, 194-195, 198-198, 200-200, 217-218, 222-223, 231-246, 251-251, 255-256, 261-262, 265-265, 277-277, 281-281, 287-288, 297-297, 300-300, 316-319, 327-327, 332-332, 337-337, 340-340, 345-345, 374-379, 392-392, 398-398, 402-404, 415-421, 426-427, 442-442, 446-447, 452-453, 458-458, 462-462, 483-483, 488-488, 490-490, 494-494, 497-497, 501-501, 506-509, 514-514, 520-520, 528-528
packages/theme/src/pager/vars.less (1)
89-94: Verify input height variables for consistencyLines 92 and 94 define the input heights for default and mini sizes. Ensure that these sizes correspond correctly to the intended design specifications.
Run the following script to check the usage of these variables across the codebase:
This will help confirm that the variables are applied consistently in the styles.
Verification successful
To gather the necessary information for a complete verification, I'll run two more scripts.
Input height variables are consistent with design specifications
The verification process confirms that the input height variables for both default and mini sizes are correctly defined and consistently used across the pager component styles:
- Default size (--tv-Pager-input-height) uses --tv-size-height-md (32px)
- Mini size (--tv-Pager-input-height-xs) uses --tv-size-height-xs (24px)
These values align with common design systems for medium and mini sizes, and their application in the styles is appropriate for maintaining consistent sizing across the pager component.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the input height variables are used consistently. # Expect: Variables should be used in the pager component styles. rg --type less "\-\-tv\-Pager\-input\-height"Length of output: 1077
Script:
#!/bin/bash # Search for definitions of --tv-size-height-md and --tv-size-height-xs rg --type less "\-\-tv\-size\-height\-(md|xs):" # Examine the context where --tv-Pager-input-height-xs is used rg --type less -C 5 "\-\-tv\-Pager\-input\-height\-xs" packages/theme/src/pager/index.lessLength of output: 716
| 'tv-Pager-sizes-num-margin-right': 'var(--ti-common-space-base, 4px)', | ||
| 'tv-Pager-mini-min-width': '18px', | ||
| 'tv-Pager-selector-padding-y': 'var(--ti-common-space-3x, 12px)', | ||
| 'tv-Pager-input-btn-padding-horizontal': '6px', | ||
| 'tv-Pager-sizes-input-hover-text-color': 'var(--ti-common-color-info-normal, #252b3a)', | ||
| 'tv-Pager-prev-next-hover-bg-color': 'var(--ti-common-color-hover-background, #f2f5fc)', | ||
| 'tv-Pager-dot-is-popup-svg-font-size': '14px', | ||
| 'tv-Pager-svg-font-size': 'var(--tv-Pager-font-size)', | ||
| 'tv-Pager-total-svg-fill-color': 'var(--ti-common-color-info-normal, #252b3a)', | ||
| 'tv-Pager-total-line-height': '24px', | ||
| 'tv-Pager-total-height': '24px', | ||
| 'tv-Pager-total-font-size': '12px', | ||
| 'tv-Pager-btn-svg-fill-color': 'var(--ti-common-color-info-normal, #252b3a)', | ||
| 'tv-Pager-next-margin-left': '6px', | ||
| 'tv-Pager-next-margin-right': '6px', | ||
| 'tv-Pager-next-padding-right': '4px', | ||
| 'tv-Pager-prev-margin-right': '4px', | ||
| 'tv-Pager-prev-margin-left': '4px', | ||
| 'tv-Pager-prev-padding-right': '6px', | ||
| 'tv-Pager-prev-padding-left': '6px', | ||
| 'tv-Pager-goto-btn-text-hover-color': 'var(--ti-common-color-text-highlight, #526ecc)', | ||
| 'tv-Pager-poplist-item-min-height': '30px', | ||
| 'tv-Pager-poplist-item-selected-bg-color': 'var(--ti-common-color-selected-background, #5e7ce0)', | ||
| 'tv-Pager-poplist-item-hover-text-color': 'var(--ti-common-color-text-highlight, #526ecc)', | ||
| 'tv-Pager-poplist-item-hover-bg-color': 'var(--ti-common-color-hover-background, #f2f5fc)', | ||
| 'tv-Pager-li-item-hover-font-weight': 'var(--ti-common-font-weight-normal)', | ||
| 'tv-Pager-li-item-hover-text-color': 'var(--ti-common-color-text-highlight, #526ecc)', | ||
| 'tv-Pager-li-border-radius': 'var(--ti-common-border-radius-normal, 2px)', | ||
| 'tv-Pager-poplist-item-padding-horizontal': 'var(--ti-common-space-6, 6px)', | ||
| 'tv-Pager-input-padding-horizontal': '0', | ||
| 'tv-Pager-input-width': '40px', | ||
| 'tv-Pager-min-width': 'auto', | ||
| 'tv-Pager-height': 'var(--ti-common-size-height-mini, 24px)', | ||
| 'tv-Pager-normal-text-padding-left': 'calc(var(--ti-common-space-base) * 3 + 2px, 14px)', | ||
| 'tv-Pager-mini-font-size': 'var(--ti-common-font-size-base, 12px)', | ||
| 'tv-Pager-input-padding-y': '10px', | ||
| 'tv-Pager-input-height': 'var(--ti-common-size-height-mini, 24px)', | ||
| 'tv-Pager-input-border-color': 'var(--ti-common-color-border, #adb0b8)', | ||
| 'tv-Pager-active-font-weight': 'var(--ti-common-font-weight-normal)', | ||
| 'tv-Pager-active-bg-color': 'var(--ti-common-color-primary-normal, #5e7ce0)', | ||
| 'tv-Pager-active-font-color': 'var(--ti-common-color-text-white, #fff)', | ||
| 'tv-Pager-primary-bg-color': 'var(--ti-common-color-primary-normal, #5e7ce0)' |
There was a problem hiding this comment.
Tip
Codebase Verification
Inconsistent naming convention detected. Consider reverting or extending the changes.
The renaming from 'ti-pager-' to 'tv-Pager-' in packages/theme/src/pager/old-theme.js appears to be an isolated change. This inconsistency may lead to potential issues:
- Other theme files (infinity-theme, aurora-theme) still use the 'ti-pager-' prefix.
- No imports or uses of
old-theme.jswere found, raising questions about its purpose and integration. - Lack of documentation or comments about this renaming process.
To address these issues:
- Either revert the changes in
old-theme.jsto maintain consistency with other theme files, or - Extend the renaming process to all relevant files in the project.
- Add documentation explaining the rationale behind the naming change if it's intentional.
Please review and clarify the intended scope of this renaming to ensure consistency across the codebase.
Analysis chain
LGTM! Verify the usage of the renamed properties.
The renaming of properties from ti- to tv- prefix is consistent across the object and does not introduce any functional changes. However, ensure that all references to these properties in the codebase have been updated to use the new naming convention.
Run the following script to verify the usage of the renamed properties:
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the old property names are not used in the codebase.
# Test: Search for the old property names. Expect: No occurrences.
rg --type js $'ti-pager-'
Length of output: 2544
Script:
#!/bin/bash
# Description: Check for new naming convention and usage of old-theme.js
# Search for the new naming convention
echo "Searching for new naming convention 'tv-Pager-':"
rg --type js 'tv-Pager-'
echo -e "\nSearching for imports or uses of old-theme.js:"
rg --type js 'old-theme\.js'
echo -e "\nSearching for comments about renaming process:"
rg --type js -i 'renaming|refactor.*pager'
Length of output: 4664
| // 分页输入框纵向内边距 | ||
| --ti-pager-input-padding-vertical: var(--ti-common-space-3x); | ||
| // 分页数字字体 | ||
| --tv-Pager-number-font-family: arial, helvetica, sans-serif; |
There was a problem hiding this comment.
Capitalize font family names for standard fonts
In line 15, the font family names are all in lowercase. According to CSS conventions, standard font family names should be capitalized to ensure consistency and prevent any potential issues with font rendering.
Apply this diff to capitalize the font family names:
--tv-Pager-number-font-family: arial, helvetica, sans-serif;
+--tv-Pager-number-font-family: Arial, Helvetica, sans-serif;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.
| --tv-Pager-number-font-family: arial, helvetica, sans-serif; | |
| --tv-Pager-number-font-family: Arial, Helvetica, sans-serif; |
packages/theme/src/pager/vars.less
Outdated
| --tv-Pager-page-item-min-width: 32px; | ||
| // 小尺寸分页页码项最小宽度 | ||
| --tv-Pager-min-width-xs: 24px; |
There was a problem hiding this comment.
Ensure consistent naming for minimum width variables
At line 70, the variable is named --tv-Pager-page-item-min-width, whereas at line 72, the variable is --tv-Pager-min-width-xs. The inconsistency in naming may lead to confusion.
For consistency, consider renaming --tv-Pager-min-width-xs to --tv-Pager-page-item-min-width-xs, aligning it with the naming convention used in line 70.
Apply this diff to rename the variable:
---tv-Pager-min-width-xs: 24px;
+--tv-Pager-page-item-min-width-xs: 24px;Remember to update any references to this variable in other parts of the codebase.
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.
| --tv-Pager-page-item-min-width: 32px; | |
| // 小尺寸分页页码项最小宽度 | |
| --tv-Pager-min-width-xs: 24px; | |
| --tv-Pager-page-item-min-width: 32px; | |
| // 小尺寸分页页码项最小宽度 | |
| --tv-Pager-page-item-min-width-xs: 24px; |
packages/theme/src/pager/vars.less
Outdated
| --tv-Pager-text-color-disabled: var(--tv-color-text-disabled); | ||
| // 分页二级文本颜色，页码项文本色 | ||
| --tv-Pager-text-color-secondary: var(--tv-color-text-secondary); | ||
| // 分页禁用状态下字体颜色 | ||
| --tv-Pager-disabled-text-color: var(--tv-color-text-disabled); |
There was a problem hiding this comment.
Consolidate duplicate variables for disabled text color
Lines 23 and 27 define two variables for the disabled text color:
--tv-Pager-text-color-disabled--tv-Pager-disabled-text-color
Both variables are assigned the same value var(--tv-color-text-disabled). This redundancy can lead to confusion and makes maintenance more difficult.
Consider consolidating these into a single variable to improve clarity and maintainability. If both are necessary, ensure that their purposes are clearly differentiated in the comments.
Apply this diff to remove the duplicate variable:
-// 分页禁用状态下字体颜色
---tv-Pager-disabled-text-color: var(--tv-color-text-disabled);Alternatively, if both variables serve different purposes, update the comments to reflect their distinct uses.
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.
| --tv-Pager-text-color-disabled: var(--tv-color-text-disabled); | |
| // 分页二级文本颜色，页码项文本色 | |
| --tv-Pager-text-color-secondary: var(--tv-color-text-secondary); | |
| // 分页禁用状态下字体颜色 | |
| --tv-Pager-disabled-text-color: var(--tv-color-text-disabled); | |
| --tv-Pager-text-color-disabled: var(--tv-color-text-disabled); | |
| // 分页二级文本颜色，页码项文本色 | |
| --tv-Pager-text-color-secondary: var(--tv-color-text-secondary); |
| --tv-Pager-input-height-xs: var(--tv-size-height-xs); | ||
| // -------跳至相关样式------ | ||
| // 跳至左侧文字左侧内边距 | ||
| --tv-Pager-goto-left-padding-left: var(--tv-space-xl); |
There was a problem hiding this comment.
Correct redundancy in variable name
The variable --tv-Pager-goto-left-padding-left in line 97 contains redundant wording with "left" appearing twice.
Simplify the variable name to improve clarity:
---tv-Pager-goto-left-padding-left: var(--tv-space-xl);
+--tv-Pager-goto-padding-left: var(--tv-space-xl);Update any references to this variable in the codebase to reflect the change.
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.
| --tv-Pager-goto-left-padding-left: var(--tv-space-xl); | |
| --tv-Pager-goto-padding-left: var(--tv-space-xl); |
gimmyhehe
changed the title
PR
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Does this PR introduce a breaking change?
Other information
Summary by CodeRabbit
New Features
Bug Fixes
Refactor