-
Notifications
You must be signed in to change notification settings - Fork 276
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
feat(popconfirm): [popconfirm] Adapting to the SMB theme #2168
Conversation
WalkthroughThe pull request introduces modifications to the Changes
Possibly related PRs
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 (
|
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.
Actionable comments posted: 3
Outside diff range and nitpick comments (5)
examples/sites/demos/pc/app/popconfirm/type-composition-api.vue (1)
40-41
: LGTM: Improved title and added messageThe changes in the script section align well with the template modifications:
- The
title
ref now contains a more generic prompt, improving reusability.- The new
message
ref provides detailed context for the confirmation prompts.These changes enhance the component's flexibility and user experience.
Consider extracting the hardcoded strings into constants or a configuration object for easier maintenance and potential internationalization:
const POPCONFIRM_STRINGS = { TITLE: '提示标题', MESSAGE: '安全组规则是推荐的,确定要删除该安全组规则吗?' } const title = ref(POPCONFIRM_STRINGS.TITLE) const message = ref(POPCONFIRM_STRINGS.MESSAGE)packages/theme/src/popconfirm/vars.less (3)
14-15
: LGTM! Consider translating the comment to English.The introduction of
--ti-popconfirm-popover-title-line-height
using a common variable improves consistency. However, for better international collaboration, consider translating the Chinese comment to English.Suggested translation:
- // 提示弹窗title字体行高 + // Line height for the popover title
16-17
: LGTM! Consider translating the comment to English.The introduction of
--ti-popconfirm-popover-content-line-height
using a common variable improves consistency. However, for better international collaboration, consider translating the Chinese comment to English.Suggested translation:
- // 提示弹窗content字体行高 + // Line height for the popover content
37-38
: LGTM! Consider translating the comment to English.The introduction of
--ti-popconfirm-popover-margin-bottom
using a common spacing variable improves consistency. However, for better international collaboration, consider translating the Chinese comment to English.Suggested translation:
- // 标题距离内容下边距 + // Margin between title and contentpackages/theme/src/popconfirm/index.less (1)
57-57
: LGTM! Consider adding a fallback value.The addition of the
line-height
property using a CSS variable is a good improvement for consistency and theming.Consider adding a fallback value for browsers that don't support CSS variables:
- line-height: var(--ti-popconfirm-popover-title-line-height); + line-height: var(--ti-popconfirm-popover-title-line-height, 1.5);
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- examples/sites/demos/pc/app/popconfirm/type-composition-api.vue (1 hunks)
- examples/sites/demos/pc/app/popconfirm/type.vue (2 hunks)
- packages/theme/src/popconfirm/index.less (2 hunks)
- packages/theme/src/popconfirm/vars.less (2 hunks)
- packages/vue/src/popconfirm/src/pc.vue (1 hunks)
Additional comments not posted (11)
examples/sites/demos/pc/app/popconfirm/type-composition-api.vue (2)
3-3
: LGTM: Consistent implementation of:message
propThe addition of the
:message
prop to alltiny-popconfirm
components is consistent and aligns with the PR objective of adapting the popconfirm component to the SMB theme. This change enhances the component by allowing for more detailed confirmation prompts.Also applies to: 9-9, 15-15, 21-21, 27-27
Line range hint
1-54
: Summary: Successful adaptation of popconfirm component to SMB themeThe changes in this file successfully adapt the popconfirm component to the SMB theme by:
- Adding a
:message
prop to alltiny-popconfirm
components in the template.- Updating the
title
ref to a more generic prompt.- Introducing a new
message
ref for detailed confirmation context.These modifications enhance the component's flexibility and user experience while maintaining consistency across different popconfirm types. The implementation follows Vue 3 Composition API best practices and aligns well with the PR objectives.
examples/sites/demos/pc/app/popconfirm/type.vue (4)
9-9
: LGTM! Consistent implementation.The addition of the
:message
prop to the "error" type popconfirm is consistent with the previous change, maintaining a uniform approach across different popconfirm types.
15-15
: LGTM! Maintaining consistency.The addition of the
:message
prop to the "warning" type popconfirm maintains the consistent implementation across different popconfirm types.
21-21
: LGTM! Comprehensive implementation.The addition of the
:message
prop to both the "success" type and custom type popconfirms completes the implementation across all popconfirm variations. This ensures a consistent user experience regardless of the popconfirm type used.Also applies to: 27-27
Line range hint
1-63
: Overall assessment: Good implementation with room for improvementThe changes successfully adapt the popconfirm component to the SMB theme by adding a message prop, enhancing user information. The implementation is consistent across all popconfirm types, which is commendable.
However, there are opportunities for improvement:
- Implement i18n for better internationalization support.
- Consider making the title and message more flexible to accommodate various use cases.
- Ensure that the changes align with the project's coding standards and best practices.
These suggestions would further enhance the component's versatility and maintainability.
packages/theme/src/popconfirm/vars.less (3)
25-25
: LGTM! Improved consistency in vertical padding.Updating
--ti-popconfirm-popover-container-padding-vertical
to usevar(--ti-common-space-base)
enhances consistency with the common spacing system. This change aligns well with the PR objective of adapting to the SMB theme.
Line range hint
1-38
: Overall, the changes look good and align with the PR objectives.The modifications to the Popconfirm component's CSS variables successfully adapt it to the SMB theme. The changes improve consistency by using common variables for line heights, spacing, and sizes. Here's a summary of the review:
- New variables for title and content line heights improve consistency.
- Updated vertical padding for the popover container enhances theme alignment.
- Increased icon size aligns with the SMB theme but requires layout verification.
- New margin variable between title and content improves spacing consistency.
To further improve the code:
- Consider translating Chinese comments to English for better international collaboration.
- Verify that the increased icon size doesn't cause any layout issues.
Great job on adapting the Popconfirm component to the SMB theme!
27-27
: LGTM! Verify layout impact of increased icon size.Updating
--ti-popconfirm-popover-icon-width
tovar(--ti-common-size-5x)
increases the icon size, which aligns with the SMB theme adaptation. However, it's important to ensure this change doesn't negatively impact the overall layout.Please verify that the increased icon size doesn't cause any layout issues or misalignments in the popconfirm component. Consider checking the following:
packages/theme/src/popconfirm/index.less (1)
62-62
: Approved, but be aware of potential breaking change.The update to use a more specific CSS variable name for the content's line-height is good for clarity. However, this change might break existing theme customizations that use the old variable name.
To ensure this change doesn't cause unexpected issues, please run the following script to check for any other uses of the old variable name:
If the script returns any results, consider keeping both variables for backward compatibility or update all occurrences.
Verification successful
Please run the following corrected script to check for any other uses of the old CSS variable name:
Please run the following corrected script to check for any other uses of the old CSS variable name:
Verification Successful.
The old CSS variable
--ti-popconfirm-popover-line-height
is not used elsewhere in the codebase. The update to--ti-popconfirm-popover-content-line-height
is safe and does not introduce any breaking changes.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for other uses of the old CSS variable name # Test: Search for the old variable name rg --type less --type css $'--ti-popconfirm-popover-line-height'Length of output: 501
Script:
#!/bin/bash # Description: Check for other uses of the old CSS variable name # Test: Search for the old variable name rg --type less --type css -e '--ti-popconfirm-popover-line-height'Length of output: 66
Script:
#!/bin/bash # Description: Check for other uses of the old CSS variable name # Test: Search for the old CSS variable name rg --type less --type css -- '--ti-popconfirm-popover-line-height'Length of output: 66
packages/vue/src/popconfirm/src/pc.vue (1)
25-27
: Clarify the impact of removing the conditional classThe conditional class binding for 'no-message' has been removed from the title div. While this simplifies the template, it may affect the component's appearance when there's no message.
- Could you please explain how this change aligns with the SMB theme adaptation mentioned in the PR objectives?
- Have you verified the visual appearance of the component both with and without a message to ensure it still looks as intended?
To help verify the impact of this change, you can run the following script to check for any related CSS rules:
This will help identify any CSS rules that might need adjustment due to the removal of the conditional class.
Verification successful
Verification Complete: No Issues Found
The removal of the
no-message
class does not impact any existing styles, as there are no CSS rules associated with it. Thetiny-popconfirm-popover__title
class remains in use and is properly referenced within the component.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for CSS rules related to the removed 'no-message' class and the title element # Search for 'no-message' class in CSS files echo "Searching for 'no-message' class in CSS files:" rg --type css 'no-message' # Search for 'tiny-popconfirm-popover__title' in CSS files echo "\nSearching for 'tiny-popconfirm-popover__title' in CSS files:" rg --type css 'tiny-popconfirm-popover__title'Length of output: 323
Script:
#!/bin/bash # Description: Verify usage of 'tiny-popconfirm-popover__title' class in HTML and JavaScript files echo "Searching for 'tiny-popconfirm-popover__title' class in HTML and JavaScript files:" rg --type html 'tiny-popconfirm-popover__title' rg --type js 'tiny-popconfirm-popover__title' # Additionally, check for related CSS rules that might affect styling echo "\nChecking for related CSS rules in CSS files:" rg --type css 'tiny-popconfirm-popover__title'Length of output: 520
@@ -1,30 +1,30 @@ | |||
<template> | |||
<div> | |||
<tiny-popconfirm :title="title" type="info"> | |||
<tiny-popconfirm :title="title" type="info" :message="message"> |
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.
LGTM! Consider i18n for scalability.
The addition of the :message
prop enhances the popconfirm component by providing more detailed information to the user, which aligns well with the PR objective of adapting to the SMB theme.
Consider wrapping the title
and message
content in i18n translations for better internationalization support:
<tiny-popconfirm :title="$t('popconfirm.info.title')" type="info" :message="$t('popconfirm.info.message')">
title: '提示标题', | ||
message: '安全组规则是推荐的,确定要删除该安全组规则吗?', |
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.
Consider internationalization and clarity improvements.
The changes to the data()
function provide more flexibility with a generic title and a detailed message. However, there are a few points to consider:
- The use of Chinese characters might limit internationalization.
- The generic title '提示标题' (which translates to "Prompt Title") might be too vague.
- The message is specific to deleting a security group rule, which might not be applicable to all use cases demonstrated in this component.
Consider the following improvements:
- Implement i18n for both title and message:
data() {
return {
title: this.$t('popconfirm.title'),
message: this.$t('popconfirm.message'),
// ...
}
}
- Make the message more generic or provide different messages for different popconfirm types:
data() {
return {
title: this.$t('popconfirm.title'),
messages: {
info: this.$t('popconfirm.info.message'),
error: this.$t('popconfirm.error.message'),
warning: this.$t('popconfirm.warning.message'),
success: this.$t('popconfirm.success.message'),
custom: this.$t('popconfirm.custom.message'),
},
// ...
}
}
Then update the template to use the appropriate message:
<tiny-popconfirm :title="title" type="info" :message="messages.info">
<!-- ... -->
</tiny-popconfirm>
This approach would provide more flexibility and clarity for different popconfirm types while supporting internationalization.
@@ -73,6 +69,7 @@ | |||
} | |||
|
|||
&__confirm-button { | |||
margin-left: 8px; |
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.
LGTM! Consider using a CSS variable for consistency.
Adding margin to the confirm button improves the layout. However, for better consistency with the design system and easier theming, consider using a CSS variable instead of a fixed pixel value.
Consider updating the code as follows:
- margin-left: 8px;
+ margin-left: var(--ti-popconfirm-confirm-button-margin-left, 8px);
Don't forget to add this new variable to your vars.less
file or wherever you define your CSS variables.
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.
margin-left: 8px; | |
margin-left: var(--ti-popconfirm-confirm-button-margin-left, 8px); |
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
:message
prop for thetiny-popconfirm
component, providing additional context for confirmation prompts.Bug Fixes
Style
popconfirm
component, including updates to line heights and margin properties for improved visual presentation.