-
Notifications
You must be signed in to change notification settings - Fork 7.1k
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
fix(@vben/web-ele): fixed some style issues in dark mode #4298
Conversation
|
WalkthroughThe changes introduce new properties to the Changes
Assessment against linked issues
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: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (17)
- docs/src/components/common-ui/vben-drawer.md (1 hunks)
- docs/src/components/common-ui/vben-modal.md (1 hunks)
- packages/@core/ui-kit/layout-ui/src/components/layout-content.vue (2 hunks)
- packages/@core/ui-kit/layout-ui/src/components/layout-sidebar.vue (1 hunks)
- packages/@core/ui-kit/layout-ui/src/vben-layout.vue (1 hunks)
- packages/@core/ui-kit/popup-ui/src/drawer/drawer-api.ts (1 hunks)
- packages/@core/ui-kit/popup-ui/src/drawer/drawer.ts (1 hunks)
- packages/@core/ui-kit/popup-ui/src/drawer/drawer.vue (2 hunks)
- packages/@core/ui-kit/popup-ui/src/modal/modal-api.ts (1 hunks)
- packages/@core/ui-kit/popup-ui/src/modal/modal.ts (1 hunks)
- packages/@core/ui-kit/popup-ui/src/modal/modal.vue (2 hunks)
- packages/@core/ui-kit/shadcn-ui/src/index.ts (1 hunks)
- packages/effects/hooks/src/use-design-tokens.ts (2 hunks)
- packages/effects/layouts/src/basic/content/content-spinner.vue (1 hunks)
- packages/effects/layouts/src/basic/layout.vue (1 hunks)
- packages/effects/layouts/src/widgets/preferences/blocks/toggle-item.vue (1 hunks)
- playground/src/router/routes/modules/examples.ts (2 hunks)
Files skipped from review due to trivial changes (2)
- packages/effects/layouts/src/widgets/preferences/blocks/toggle-item.vue
- playground/src/router/routes/modules/examples.ts
Additional comments not posted (27)
packages/@core/ui-kit/shadcn-ui/src/index.ts (1)
2-2
: LGTM!The code change is approved. Exporting the
Slot
component from theradix-vue
package is a good addition, as it provides developers with easy access to this commonly used component from the Radix UI library.packages/effects/layouts/src/basic/content/content-spinner.vue (1)
11-11
: Simplification of Spinner ComponentThe removal of the
overlayStyle
prop simplifies the component, which aligns with the PR's objective to fix style issues by potentially standardizing the appearance. However, this change reduces the flexibility for custom styling. Ensure that this simplification does not negatively impact other areas where theVbenSpinner
is used, especially in contexts that might have relied on custom styling provided byoverlayStyle
.Verification successful
No Negative Impact from
overlayStyle
RemovalThe removal of the
overlayStyle
prop from theVbenSpinner
component does not negatively impact other components or styles. The search results confirm that no other instances ofVbenSpinner
rely on theoverlayStyle
prop, ensuring that the simplification aligns with the PR's objectives without causing issues elsewhere.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the removal of `overlayStyle` does not negatively impact other components or styles. # Test: Search for other occurrences of `VbenSpinner` to ensure they do not rely on `overlayStyle`. rg --type vue -A 5 $'VbenSpinner'Length of output: 60
Script:
#!/bin/bash # Description: Verify that the removal of `overlayStyle` does not negatively impact other components or styles. # Find all .vue files and search for occurrences of `VbenSpinner`. fd --extension vue --exec rg 'VbenSpinner' {}Length of output: 269
packages/@core/ui-kit/layout-ui/src/components/layout-content.vue (2)
58-60
: Approve the use of theSlot
component in the template.The introduction of the
Slot
component enhances styling flexibility, particularly for overlays in dark mode. Ensure that theoverlayStyle
is correctly computed and applied as intended.Verification successful
overlayStyle
is correctly computed and applied in the codebase.The
overlayStyle
is defined inuse-content-style.ts
and used inlayout-content.vue
, confirming its correct application in the template changes. No further action is required.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correct application of overlayStyle. # Test: Search for the overlayStyle usage in the project to ensure it's correctly applied. rg "overlayStyle" --type vueLength of output: 56
Script:
#!/bin/bash # Description: Verify the correct application of overlayStyle. # Test: Search for the overlayStyle usage in the project without specifying file type. rg "overlayStyle" # Additionally, use ast-grep to search for overlayStyle in Vue files. ast-grep --lang vue --pattern '$_ = overlayStyle'Length of output: 944
8-8
: Verify the import path for theSlot
component.Ensure that the
Slot
component is correctly imported from@vben-core/shadcn-ui
. This is crucial for the new functionality to work as expected.Verification successful
Import path for the
Slot
component is correct.The
Slot
component is correctly imported from@vben-core/shadcn-ui
, as it is re-exported fromradix-vue
within the package. This setup ensures that the component is available for use as intended. No further action is needed.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence of the Slot component in the specified package. # Test: Search for the Slot component definition in the specified package. fd Slot.vue packages/@vben-core/shadcn-ui/componentsLength of output: 182
Script:
#!/bin/bash # Description: Search for the Slot.vue file across the entire repository to locate its correct path. # Test: Use fd to find the Slot.vue file. fd Slot.vueLength of output: 13
Script:
#!/bin/bash # Description: Search for the Slot component definition in JavaScript or TypeScript files across the repository. # Test: Use rg to find the Slot component definition. rg 'Slot' --type js --type ts --type vue -A 3Length of output: 73
Script:
#!/bin/bash # Description: Search for the Slot component definition in JavaScript and TypeScript files across the repository. # Test: Use rg to find the Slot component definition, focusing on export statements. rg 'export.*Slot' --type js --type ts -A 3Length of output: 139
packages/@core/ui-kit/popup-ui/src/drawer/drawer.ts (1)
53-62
: Approve the addition ofshowCancelButton
andshowConfirmButton
.The new properties
showCancelButton
andshowConfirmButton
have been added to theDrawerProps
interface. These properties are optional and default totrue
, which is a good practice as it maintains backward compatibility. The documentation comments are clear and informative.It's important to verify that the integration of these new properties in the UI does not introduce any new style issues, especially in dark mode, as the PR aims to address style inconsistencies.
packages/@core/ui-kit/popup-ui/src/drawer/drawer-api.ts (1)
39-40
: LGTM!The code changes are approved for the following reasons:
- The addition of
showCancelButton
andshowConfirmButton
properties to thedefaultState
object enhances the functionality of the drawer by allowing it to display cancel and confirm buttons by default.- This modification improves user experience by providing users with more interactive options when the drawer is open, facilitating user actions directly within the drawer interface.
- The changes are consistent with the existing code and follow the same naming convention and initialization pattern as other properties in the
defaultState
object.- The changes do not introduce any syntax errors or type mismatches.
packages/@core/ui-kit/popup-ui/src/modal/modal-api.ts (1)
43-44
: Approve the addition ofshowCancelButton
andshowConfirmButton
.The changes to introduce
showCancelButton
andshowConfirmButton
with default values set totrue
are aligned with the PR's objectives to enhance user interaction within modals. This allows for a more flexible and user-friendly interface where developers can opt to show or hide these buttons based on the application's needs.Please ensure that these new properties are properly integrated with the UI components that utilize the
ModalApi
class, particularly in contexts where the modal's appearance is critical, such as in dark mode scenarios described in the issue.docs/src/components/common-ui/vben-drawer.md (2)
77-77
: LGTM!The new
showCancelButton
property is a useful addition to the component API. It allows the user to control the visibility of the cancel button, enhancing the component's customizability.
78-78
: LGTM!Similar to the
showCancelButton
property, the newshowConfirmButton
property is a valuable addition to the component API. It allows the user to control the visibility of the confirm button, further enhancing the component's flexibility.docs/src/components/common-ui/vben-modal.md (1)
87-88
: Document the new properties clearly and ensure alignment with PR objectives.The addition of
showCancelButton
andshowConfirmButton
properties to theuseVbenModal
configuration is well-documented. These properties allow developers to control the visibility of the modal's buttons, enhancing flexibility and user interaction based on specific use cases.However, it's important to ensure that these changes align with the overall objectives of the PR, which focuses on fixing style issues in dark mode. If these properties impact the modal's appearance in dark mode, this should be explicitly mentioned to clarify their relevance to the PR's goals.
packages/@core/ui-kit/popup-ui/src/drawer/drawer.vue (2)
57-58
: Effective use of reactive properties for UI control.The introduction of
showCancelButton
andshowConfirmButton
usingusePriorityValue
effectively enhances the dynamic control of UI elements based on the component's state or props. This change aligns well with the PR's objectives to improve UI responsiveness and control.
172-176
: Improved UI flexibility and functionality with conditional rendering.The conditional rendering of the cancel and confirm buttons based on
showCancelButton
andshowConfirmButton
is correctly implemented. This approach not only enhances the UI's responsiveness but also provides flexibility through the use of slots for button labels, allowing easy customization or localization. The binding of event handlers todrawerApi
methods ensures proper functionality.Also applies to: 182-186
packages/@core/ui-kit/layout-ui/src/components/layout-sidebar.vue (2)
220-222
: LGTM!The code changes are approved. The conditional locking of the sidebar based on the
isSidebarMixed
prop aligns with the expected behavior described in the AI-generated summary.
228-230
: LGTM!The code changes are approved. The conditional unlocking of the sidebar based on the
isSidebarMixed
prop aligns with the expected behavior described in the AI-generated summary.packages/@core/ui-kit/popup-ui/src/modal/modal.vue (3)
72-72
: LGTM!The code changes are approved. The new
showCancelButton
property is correctly initialized using theusePriorityValue
function to control the visibility of the cancel button.
73-73
: LGTM!The code changes are approved. The new
showConfirmButton
property is correctly initialized using theusePriorityValue
function to control the visibility of the confirm button.
Line range hint
243-260
: LGTM!The code changes are approved. The rendering of the cancel and confirm buttons is now conditional based on the
showCancelButton
andshowConfirmButton
properties, respectively. These changes enhance the modal's flexibility by allowing developers to control the presence of these buttons dynamically, improving the component's configurability and user interaction capabilities.packages/effects/layouts/src/basic/layout.vue (1)
298-299
: Simplified rendering logic for the loading spinner.The changes to the
LayoutContentSpinner
component simplify the template by removing the:overlay-style
prop. This should enhance clarity and maintainability. The condition for displaying the spinner remains unchanged, ensuring that the loading behavior is preserved.Please verify the visual impact of these changes, especially in dark mode, to ensure that the removal of the
:overlay-style
prop does not introduce new style issues.packages/effects/hooks/src/use-design-tokens.ts (7)
163-163
: Addition ofusePreferences
hook.The addition of the
usePreferences
hook allows the function to access user preferences, specifically the theme setting. This is crucial for applying the correct styles based on the theme.
166-168
: Introduction ofgetCssVariableValueRaw
function.This new helper function simplifies the process of fetching CSS variable values directly from the root styles. It enhances code readability and maintainability by centralizing the retrieval logic.
171-171
: Refactor ofgetCssVariableValue
to use the new helper function.Refactoring this function to use
getCssVariableValueRaw
improves the separation of concerns by isolating the raw value retrieval. This change makes the code cleaner and easier to manage.
188-188
: Dynamic handling of--el-border-color-hover
.The assignment of
--el-border-color-hover
based on the accent color in dark mode is a targeted fix that addresses the hover style issues mentioned in the PR description. This ensures that hover styles are visually consistent in dark mode.
218-220
: Conditional assignment based on theme for--el-color-primary-light-9
.This conditional logic ensures that the color value adapts to the theme, using the accent color in dark mode. It's a smart use of reactive programming to handle theme changes dynamically.
239-241
: Use of--accent
for--el-fill-color-light
.Applying the accent color to
--el-fill-color-light
enhances the thematic consistency across the UI components, aligning with the dark mode adjustments.
241-241
: Consistent text color assignments.The use of
--foreground
for text colors ensures consistency and readability across different themes. This is a good practice in theme design to maintain visual harmony.packages/@core/ui-kit/layout-ui/src/vben-layout.vue (2)
537-538
: Review the changes to the overlay template structure.The changes to the overlay template structure involve moving from a scoped slot to a non-scoped slot. This simplifies the slot usage but removes the direct style binding from the parent component. It's important to ensure that this change does not negatively impact the visual presentation and behavior of the overlay, especially in dark mode contexts where style issues are being addressed.
Verify that the removal of
overlayStyle
binding does not introduce any regressions in the overlay's appearance and functionality. This can be done by visually inspecting the overlay in different scenarios and checking if any related styles are defined elsewhere that might compensate for the removed binding.
537-538
: Approve the changes tohandleHeaderToggle
.The implementation of
handleHeaderToggle
is straightforward and correctly handles the sidebar visibility based on the device type. It properly uses Vue's event system to manage state changes, which is crucial for maintaining consistency across the component.Ensure that the integration of this function with other components (especially those related to the sidebar and mobile responsiveness) is tested thoroughly to avoid any regressions.
/** | ||
* 是否显示取消按钮 | ||
* @default true | ||
*/ | ||
showCancelButton?: boolean; | ||
/** | ||
* 是否显示确认按钮 | ||
* @default true | ||
*/ | ||
showConfirmButton?: boolean; |
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.
Approve the addition of showCancelButton
and showConfirmButton
.
The changes to the ModalProps
interface introduce showCancelButton
and showConfirmButton
properties, enhancing the modal's flexibility and customization. Both properties are optional and default to true
, ensuring backward compatibility.
It's recommended to add unit tests to verify that the modal behaves correctly with these properties set to different values. Would you like assistance in setting up these tests?
Description
fixed #4295
Type of change
Please delete options that are not relevant.
pnpm-lock.yaml
unless you introduce a new test example.Checklist
pnpm run docs:dev
command.pnpm test
.feat:
,fix:
,perf:
,docs:
, orchore:
.Summary by CodeRabbit
New Features
Slot
component for improved flexibility in UI design.Bug Fixes
Documentation
Chores