refactor(theme): [popeditor] refactor popeditor theme vars #2298
Conversation
WalkthroughThe pull request introduces significant updates to the Changes
Possibly related PRs
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: 12
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- packages/theme/src/popeditor/index.less (11 hunks)
- packages/theme/src/popeditor/vars.less (1 hunks)
🧰 Additional context used
🔇 Additional comments (17)
packages/theme/src/popeditor/vars.less (1)
13-39: Consistent renaming of CSS variables and mixinThe changes in this file show a systematic update of CSS variable names from the
--ti-prefix to--tv-. This change appears to be part of a larger refactoring effort to standardize variable naming across the project. The mixin name has also been updated to.inject-Popeditor-vars(), which aligns with the new naming convention.A few points to consider:
- The consistency in renaming is commendable and should help maintain a uniform codebase.
- The comments for each variable have been retained, which is good for code readability and maintainability.
- Some variable values have been updated to use the new
--tv-prefix, ensuring internal consistency.To ensure that these changes don't introduce any unintended side effects, please run the following verification script:
This script will help identify any inconsistencies in the renaming process across the codebase.
packages/theme/src/popeditor/index.less (16)
23-23: Use of new mixin.inject-Popeditor-vars()Including
.inject-Popeditor-vars();ensures that the component correctly injects the updated Popeditor variables, aligning with the new theming approach.
45-45: Update icon fill color to use theme variableChanging to
fill: var(--tv-Popeditor-icon-color-disabled);aligns the disabled icon color with the new theme variables.
56-56: Update icon fill color for disabled stateConsistently using
fill: var(--tv-Popeditor-icon-color-disabled);ensures the disabled icons adhere to the theme.
82-82: Update icon hover fill color to theme variableSetting
fill: var(--tv-Popeditor-icon-color-hover);for hover states keeps the icon colors consistent with the theme.
92-95: Use theme variables for icon stylingApplying theme variables for
height,line-height,font-size, andfillimproves consistency and maintainability.
99-99: Update icon hover fill colorConsistently using
fill: var(--tv-Popeditor-icon-color-hover);for hover effects aligns with the theme.
127-127: Set text alignment to left for search labelsEnsuring
text-align: left;for.@{popeditor-prefix-cls}__search-labelmaintains consistent label alignment.
130-130: Use theme variable for search item widthApplying
width: var(--tv-Popeditor-search-item-width);promotes consistency and easy theming.
182-182: Reset padding for tabs headSetting
padding: 0;ensures there is no unintended spacing in the tabs header.
184-184: Set tabs head height using theme variableUsing
height: var(--tv-Popeditor-tabs-li-height);promotes consistent sizing across components.
189-189: Use theme variable for tabs list heightApplying
height: var(--tv-Popeditor-tabs-li-height);ensures uniformity in tab dimensions.
201-202: Use theme variables for line-height and font-size in tabsApplying
line-heightandfont-sizetheme variables promotes consistency in text styling.
205-207: Apply theme variables for selected tab stylingUsing variables for
color,border-bottom, andfont-weightensures consistent highlighting of selected tabs.
236-236: Ensure visibility of input prefixSetting
visibility: visible;for.tiny-input__prefixensures the prefix icon is displayed as intended.
240-240: Hide input suffix as intendedApplying
visibility: hidden;to.tiny-input__suffixcorrectly hides the suffix element.
251-251: Include Popeditor variables in dialog boxAdding
.inject-Popeditor-vars();ensures the dialog box uses the updated theme variables.
|
|
||
| .@{input-prefix-cls} { | ||
| &.@{popeditor-prefix-cls}-readonly .@{input-prefix-cls}__inner { | ||
| padding-left: var(--ti-popeditor-input-padding-left); | ||
| padding-left: 8px; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using a variable for padding-left
Hardcoding padding-left: 8px; reduces flexibility. Using a variable allows for easier theming and future adjustments.
| @@ -122,21 +121,21 @@ | |||
| display: flex; | |||
| justify-content: flex-end; | |||
| align-items: center; | |||
| margin-bottom: var(--ti-popeditor-search-item-margin-bottom); | |||
| margin-bottom: 24px; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using a variable for margin-bottom
Using margin-bottom: 24px; directly may limit flexibility. A variable would allow for easier theme adjustments.
| } | ||
|
|
||
| &:nth-child(2n + 1) { | ||
| padding-right: var(--ti-popeditor-search-item-padding-right); | ||
| padding-right: 16px; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using a variable for padding-right
Hardcoding padding-right: 16px; may reduce adaptability. A variable would enhance theme flexibility.
| } | ||
|
|
||
| &:nth-child(2n) { | ||
| padding-left: var(--ti-popeditor-search-item-padding-left); | ||
| padding-left: 16px; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using a variable for padding-left
Using a variable instead of padding-left: 16px; can improve maintainability and theming capabilities.
| text-align: right; | ||
| margin: 0 auto 24px; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using variables for text-align and margin
Replacing text-align: right; and margin: 0 auto 24px; with theme variables can enhance consistency.
| padding: 0 32px 0 0; | ||
| color: var(--tv-Popeditor-tabs-text-color); | ||
| min-width: 90px; | ||
| height: var(--ti-popeditor-tabs-li-height); | ||
| line-height: var(--ti-popeditor-tabs-li-line-height); | ||
| font-size: var(--ti-popeditor-tabs-font-size); | ||
| margin-right: var(--ti-popeditor-tabs-li-margin-right); | ||
| height: var(--tv-Popeditor-tabs-li-height); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using variables for padding and min-width in tabs
Using variables instead of padding: 0 32px 0 0; and min-width: 90px; can enhance maintainability and theming flexibility.
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| .@{popeditor-prefix-cls}__tabs-body { | ||
| .tabs-body-item { | ||
| padding: var(--ti-popeditor-tabs-body-item-padding-top) var(--ti-popeditor-tabs-body-item-padding-horizontal) | ||
| var(--ti-popeditor-tabs-body-item-padding-bottom); | ||
| padding: 16px 0 0; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using a variable for padding in tab body
Replacing padding: 16px 0 0; with a theme variable can improve maintainability.
|
|
||
| .@{pager-prefix-cls} { | ||
| padding-top: var(--ti-popeditor-pager-padding-top); | ||
| padding-top: 12px; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using a variable for padding-top in pager
Using padding-top: 12px; directly may reduce flexibility. A variable would allow for consistent theming.
| .tiny-input__inner { | ||
| padding-left: 28px; | ||
| padding-right: 8px; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using variables for padding in .tiny-input__inner
Hardcoding padding-left: 28px; and padding-right: 8px; limits theming capabilities. Variables would enhance flexibility.
| text-align: right; | ||
| padding-top: 24px; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using a variable for padding-top in dialog footer
Using padding-top: 24px; directly may limit flexibility. A variable allows for better theme customization.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
examples/sites/demos/pc/app/popeditor/size-composition-api.vue (1)
Line range hint
21-28: LGTM! Consider reordering for consistency.The reordering of the
tiny-popeditorcomponents doesn't affect functionality and is acceptable. However, for better consistency and readability, consider arranging the components in a logical order based on their sizes.You might want to consider the following order:
- mini
- small
- default (no size specified)
- medium
This would provide a clear visual progression from smallest to largest size.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- examples/sites/demos/pc/app/popeditor/size-composition-api.vue (1 hunks)
- examples/sites/demos/pc/app/popeditor/size.vue (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- examples/sites/demos/pc/app/popeditor/size.vue
🧰 Additional context used
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