-
Notifications
You must be signed in to change notification settings - Fork 278
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.
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
refactor(theme): refactor business comp theme vars #2223
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -17,19 +17,19 @@ | |||||
@logout-prefix-cls: ~'@{css-prefix}logout'; | ||||||
|
||||||
.@{user-contact-prefix-cls} { | ||||||
.component-css-vars-user-account(); | ||||||
.inject-UserAccount-vars(); | ||||||
|
||||||
&__main { | ||||||
.user-account-pop { | ||||||
padding: var(--ti-user-account-padding-vertical) var(--ti-user-account-padding-horizontal); | ||||||
padding: 0 8px; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Consider using CSS variables for padding to maintain theme flexibility. While the fixed padding of Here's a suggested modification: - padding: 0 8px;
+ padding: var(--tv-UserAccount-padding-vertical, 0) var(--tv-UserAccount-padding-horizontal, 8px); This change would allow for easy customization while providing default values. 📝 Committable suggestion
Suggested change
|
||||||
} | ||||||
|
||||||
.user-account-custom { | ||||||
line-height: var(--ti-user-account-line-height); | ||||||
line-height: var(--tv-UserAccount-line-height); | ||||||
} | ||||||
|
||||||
.@{logout-prefix-cls} { | ||||||
line-height: var(--ti-user-account-line-height); | ||||||
line-height: var(--tv-UserAccount-line-height); | ||||||
} | ||||||
} | ||||||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codebase verification
Old mixin
component-css-vars-amount
is still in use.packages/theme/src/card-group/vars.less
packages/theme/src/card-group/index.less
packages/theme/src/card/vars.less
packages/theme/src/card/index.less
🔗 Analysis chain
Approve mixin change with verification.
The change from
.component-css-vars-amount()
to.inject-Amount-vars()
improves naming specificity. However, ensure this change is consistent across the codebase and doesn't break existing styles.Run the following script to check for any remaining usage of the old mixin:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
Length of output: 861
Script:
Length of output: 318