refactor(theme): [popover] refactor popover theme vars #2259
Conversation
WalkthroughThe changes involve a comprehensive update to the CSS styles and variable definitions for the popover component in the Changes
Possibly related PRs
Suggested labels
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: 4
🧹 Outside diff range and nitpick comments (1)
packages/theme/src/popover/vars.less (1)
31-33: LGTM! Consider translating the Chinese comment.The changes to the placement margin variables look good:
- Renaming
--ti-popover-placement-margin-verticalto--tv-Popover-placement-margin-yis consistent with the new naming convention and uses a more concise 'y' suffix.- Adding
--tv-Popover-placement-margin-xprovides better control over horizontal margins.Consider translating the Chinese comment on line 32 to English for consistency with the rest of the codebase. For example:
// Horizontal margin for the popover arrow
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- packages/theme/src/popover/index.less (3 hunks)
- packages/theme/src/popover/vars.less (1 hunks)
🧰 Additional context used
🔇 Additional comments (10)
packages/theme/src/popover/vars.less (10)
15-15: LGTM! Consistent with new naming convention.The variable name change from
--ti-popover-bg-colorto--tv-Popover-bg-coloris consistent with the new naming convention. The value remains unchanged, preserving the existing functionality.
17-17: LGTM! Consistent naming update.The variable name change from
--ti-popover-text-colorto--tv-Popover-text-colorfollows the new naming convention consistently. The value remains unchanged, maintaining the existing functionality.
19-19: LGTM! Consistent with new naming pattern.The variable name change from
--ti-popover-border-radiusto--tv-Popover-border-radiusadheres to the new naming convention. The value remains unchanged, preserving the existing styling.
21-23: LGTM! Consistent naming updates for font properties.The variable name changes for font size and line height (from
--ti-popover-font-sizeto--tv-Popover-font-sizeand from--ti-popover-line-heightto--tv-Popover-line-height) are consistent with the new naming convention. The values remain unchanged, maintaining the existing typography styles.
25-29: LGTM! Consistent naming updates for title properties.The variable name changes for title-related properties (text color, font size, and weight) are all consistent with the new naming convention:
--ti-popover-title-text-colorto--tv-Popover-title-text-color--ti-popover-title-font-sizeto--tv-Popover-title-font-size--ti-popover-title-weightto--tv-Popover-title-weightThe values remain unchanged, preserving the existing title styling.
35-35: LGTM! Consistent naming update for arrow border color.The variable name change from
--ti-popover-placement-arrow-after-border-colorto--tv-Popover-placement-arrow-after-border-coloradheres to the new naming convention. The value remains unchanged, maintaining the existing arrow styling.
37-37: LGTM! Consistent naming update for box shadow.The variable name change from
--ti-popover-box-shadowto--tv-Popover-box-shadowis in line with the new naming convention. The value remains unchanged, preserving the existing shadow effect.
39-41: LGTM! Consistent and concise naming updates for padding.The variable name changes for padding are consistent with the new naming convention and improve conciseness:
--ti-popover-padding-verticalto--tv-Popover-padding-y--ti-popover-padding-horizontalto--tv-Popover-padding-xThe values remain unchanged, maintaining the existing padding styles.
Line range hint
1-41: Overall, excellent refactoring of popover theme variables.The changes in this file consistently update the naming convention for CSS variables, improving readability and maintainability. The new prefix
--tv-and the use of PascalCase for "Popover" create a more standardized approach. All functionality appears to be preserved as the variable values remain unchanged.Key improvements:
- Consistent naming convention across all variables
- More descriptive function name (
inject-Popover-vars)- Addition of a horizontal margin variable for better control
- Use of shorter suffixes ('y' and 'x') for directional properties
Great job on this refactoring effort!
13-13: LGTM! Consider verifying naming consistency across components.The function name change from
.component-css-vars-popover()to.inject-Popover-vars()looks good. It follows a new naming convention that seems more descriptive and action-oriented.To ensure consistency across the codebase, please run the following script:
This will help identify if other components have been updated similarly or if this is the only occurrence.
✅ Verification successful
Verified: Naming consistency confirmed across all components.
The renaming to
.inject-Popover-vars()aligns with the established naming convention used consistently across all components. No inconsistencies were found.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for consistency in naming of similar functions across components # Test: Search for similar function names in other component files rg --type less -g 'packages/theme/src/*/vars.less' '^\.(component-css-vars-|inject-).+-vars\(\)'Length of output: 3067
| font-weight: var(--ti-popover-title-weight); | ||
| color: var(--tv-Popover-title-text-color); | ||
| font-size: var(--tv-Popover-title-font-size); | ||
| font-weight: var(--tv-Popover-title-weight); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider renaming --tv-Popover-title-weight to --tv-Popover-title-font-weight for consistency
To maintain consistent naming conventions across your theme variables, consider renaming --tv-Popover-title-weight to --tv-Popover-title-font-weight, matching the pattern used for other font-related variables like --tv-Popover-title-font-size.
| background: var(--tv-Popover-bg-color); | ||
| color: var(--tv-Popover-text-color); | ||
| border-radius: var(--tv-Popover-border-radius); | ||
| border: 1px solid transparent; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using theme variables for border properties for consistency
Currently, the border property is set to 1px solid transparent. To maintain consistency and enhance theming flexibility, consider using theme variables for the border width and color.
You could define variables like --tv-Popover-border-width and --tv-Popover-border-color and update the code as follows:
-border: 1px solid transparent;
+border: var(--tv-Popover-border-width) solid var(--tv-Popover-border-color);Ensure these variables are defined in your theming variables file to allow for easier customization and maintenance.
📝 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.
| border: 1px solid transparent; | |
| border: var(--tv-Popover-border-width) solid var(--tv-Popover-border-color); |
| line-height: 1; | ||
| margin-bottom: var(--ti-popover-title-padding-bottom); | ||
| margin-bottom: 8px; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace hardcoded margin-bottom value with a theme variable
Currently, the margin-bottom for the popover title is set to 8px. To maintain consistency and theming flexibility, consider using a theme variable for this value.
For example:
-margin-bottom: 8px;
+margin-bottom: var(--tv-Popover-title-margin-bottom);Ensure that --tv-Popover-title-margin-bottom is defined in your variables file with the desired value.
Committable suggestion was skipped due to low confidence.
| border-width: 6px; | ||
| } | ||
|
|
||
| &[x-placement^='top'] { | ||
| margin-bottom: var(--ti-popover-placement-margin-vertical); | ||
| margin-bottom: var(--tv-Popover-placement-margin-y); | ||
|
|
||
| .popper__arrow { | ||
| bottom: calc(var(--ti-popover-arrow-height) * -1); | ||
| bottom: -8px; | ||
| left: 50%; | ||
| border-top-color: var(--ti-popover-arrow-border-color); | ||
| border-top-color: transparent; | ||
| border-bottom-width: 0; | ||
| border-top-width: var(--ti-popover-arrow-height); | ||
| border-top-width: 8px; | ||
|
|
||
| &::after { | ||
| &:after { | ||
| bottom: 1px; | ||
| margin-left: -6px; | ||
| border-top-color: var(--ti-popover-placement-arrow-after-border-color); | ||
| border-top-color: var(--tv-Popover-placement-arrow-after-border-color); | ||
| border-bottom-width: 0; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| &[x-placement^='bottom'] { | ||
| margin-top: var(--ti-popover-placement-margin-vertical); | ||
| margin-top: var(--tv-Popover-placement-margin-y); | ||
|
|
||
| .popper__arrow { | ||
| top: calc(var(--ti-popover-arrow-height) * -1); | ||
| top: -8px; | ||
| left: 50%; | ||
| border-top-width: 0; | ||
| border-bottom-color: var(--ti-popover-arrow-border-color); | ||
| border-bottom-width: var(--ti-popover-arrow-height); | ||
| border-bottom-color: transparent; | ||
| border-bottom-width: 8px; | ||
|
|
||
| &::after { | ||
| &:after { | ||
| top: 1px; | ||
| margin-left: -6px; | ||
| border-top-width: 0; | ||
| border-bottom-color: var(--ti-popover-placement-arrow-after-border-color); | ||
| border-bottom-color: var(--tv-Popover-placement-arrow-after-border-color); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| &[x-placement^='right'] { | ||
| margin-left: var(--ti-popover-placement-margin-horizontal); | ||
| margin-left: var(--tv-Popover-placement-margin-x); | ||
|
|
||
| .popper__arrow { | ||
| top: 50%; | ||
| left: calc(var(--ti-popover-arrow-height) * -1); | ||
| border-right-color: var(--ti-popover-arrow-border-color); | ||
| left: -8px; | ||
| border-right-color: transparent; | ||
| border-left-width: 0; | ||
| border-right-width: var(--ti-popover-arrow-height); | ||
| border-right-width: 8px; | ||
|
|
||
| &::after { | ||
| &:after { | ||
| bottom: -6px; | ||
| left: 1px; | ||
| border-right-color: var(--ti-popover-placement-arrow-after-border-color); | ||
| border-right-color: var(--tv-Popover-placement-arrow-after-border-color); | ||
| border-left-width: 0; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| &[x-placement^='left'] { | ||
| margin-right: var(--ti-popover-placement-margin-horizontal); | ||
| margin-right: var(--tv-Popover-placement-margin-x); | ||
|
|
||
| .popper__arrow { | ||
| top: 50%; | ||
| right: calc(var(--ti-popover-arrow-height) * -1); | ||
| right: -8px; | ||
| border-right-width: 0; | ||
| border-left-color: var(--ti-popover-arrow-border-color); | ||
| border-left-width: var(--ti-popover-arrow-height); | ||
| border-left-color: transparent; | ||
| border-left-width: 8px; | ||
|
|
||
| &::after { | ||
| &:after { | ||
| right: 1px; | ||
| bottom: -6px; | ||
| margin-left: -6px; | ||
| border-right-width: 0; | ||
| border-left-color: var(--ti-popover-placement-arrow-after-border-color); | ||
| border-left-color: var(--tv-Popover-placement-arrow-after-border-color); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace hardcoded values with theme variables for better theming support
There are multiple instances of hardcoded pixel values in the arrow and placement-specific styles, such as border-width: 6px;, border-top-width: 8px;, margin-bottom: 8px;, etc. To improve consistency and enable easier theming customization, consider replacing these hardcoded values with theme variables.
For example, define theme variables like:
--tv-Popover-arrow-border-width--tv-Popover-arrow-border-top-width--tv-Popover-placement-margin--tv-Popover-arrow-offset
Update your code accordingly. For instance:
-/* In general arrow styles */
-border-width: 6px;
+border-width: var(--tv-Popover-arrow-border-width);
-/* In placement-specific styles */
-margin-bottom: var(--tv-Popover-placement-margin-y);
+margin-bottom: var(--tv-Popover-placement-margin);
-bottom: -8px;
+bottom: calc(-1 * var(--tv-Popover-arrow-offset));
-border-top-width: 8px;
+border-top-width: var(--tv-Popover-arrow-border-top-width);Ensure that these variables are defined in your variables file (vars.less) and set appropriate values. This change will enhance the flexibility of your component's styling and maintain consistent theming across the application.
Committable suggestion was skipped due to low confidence.
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