fix: improve the dialog and drawer scrollbar experience, fix internal click failure problems and warnings #4391
Conversation
… click failure problems and warnings
|
WalkthroughThe pull request introduces several changes across various components and utility files in the codebase. Key modifications include the addition of new utility functions for scrollbar width calculation, enhancements to modal and drawer components for better focus management, and updates to CSS for pointer event handling. The changes aim to improve user interaction and component behavior, particularly in modal dialogs and overlays. Changes
Assessment against linked issues
Possibly related PRs
Poem
Tip OpenAI O1 model for chat
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: 1
Outside diff range and nitpick comments (3)
packages/@core/base/shared/src/utils/dom.ts (1)
54-71: LGTM! Consider caching the scrollbar width and handling edge cases.The
getScrollbarWidthfunction correctly calculates the scrollbar width by measuring the difference between the outer width and inner width of a temporary scrollable div element. The implementation follows a common approach and is well-structured.To further optimize the function, consider the following suggestions:
Cache the scrollbar width value:
- Store the calculated scrollbar width in a variable to avoid unnecessary DOM manipulations on subsequent calls.
- This can improve performance, especially if the function is called frequently.
Handle edge cases:
- Check if the browser supports the required DOM APIs before performing the calculations.
- Handle scenarios where the scrollbar width is zero or the temporary div elements cannot be created or appended to the DOM.
- This will make the function more robust and prevent potential errors in different browser environments.
scripts/turbo-run/src/run.ts (1)
Line range hint
1-54: Consider removing the commented-out code.The file contains commented-out code for filtering app packages (lines 52-70). If this code is no longer needed, it is recommended to remove it to improve code readability and maintainability.
Removing unused code helps keep the codebase clean and reduces confusion for future developers working on the project.
packages/@core/ui-kit/popup-ui/src/drawer/drawer.vue (1)
91-96: LGTM, but fix the typo in the function name.The
handerOpenAutoFocusfunction correctly prevents the default behavior based on theopenAutoFocusproperty to control the auto focus when the drawer opens.However, there is a typo in the function name. Please rename it to
handleOpenAutoFocusfor consistency and readability.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
Files selected for processing (19)
- packages/@core/base/design/src/css/global.css (1 hunks)
- packages/@core/base/shared/src/utils/dom.ts (1 hunks)
- packages/@core/composables/src/index.ts (1 hunks)
- packages/@core/composables/src/use-scroll-lock.ts (1 hunks)
- packages/@core/ui-kit/layout-ui/src/vben-layout.vue (2 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 (3 hunks)
- packages/@core/ui-kit/popup-ui/src/drawer/use-drawer.ts (1 hunks)
- packages/@core/ui-kit/popup-ui/src/modal/modal.vue (3 hunks)
- packages/@core/ui-kit/popup-ui/src/modal/use-modal.ts (1 hunks)
- packages/@core/ui-kit/shadcn-ui/build.config.ts (1 hunks)
- packages/@core/ui-kit/shadcn-ui/package.json (1 hunks)
- packages/@core/ui-kit/shadcn-ui/src/components/ui/dialog/DialogContent.vue (3 hunks)
- packages/@core/ui-kit/shadcn-ui/src/components/ui/dialog/DialogOverlay.vue (1 hunks)
- packages/@core/ui-kit/shadcn-ui/src/components/ui/sheet/SheetContent.vue (3 hunks)
- packages/@core/ui-kit/shadcn-ui/src/components/ui/sheet/SheetOverlay.vue (1 hunks)
- packages/effects/layouts/src/basic/tabbar/tabbar.vue (2 hunks)
- scripts/turbo-run/src/run.ts (1 hunks)
Files skipped from review due to trivial changes (1)
- packages/@core/ui-kit/shadcn-ui/build.config.ts
Additional comments not posted (34)
packages/@core/composables/src/index.ts (1)
5-5: LGTM!The change exports the
use-scroll-lockmodule, making it available for use in other parts of the application. This is a straightforward addition that does not affect the logic of other modules.The
use-scroll-lockcomposable likely provides scroll locking functionality to manage scroll behavior in specific UI contexts, which can enhance the user experience.packages/@core/ui-kit/shadcn-ui/src/components/ui/dialog/DialogOverlay.vue (3)
1-5: LGTM!The usage of the
useScrollLockcomposable is a good practice to prevent scrolling the background content when the dialog is open. The composable is imported from the project-specific@vben-core/composablespackage, which suggests it is a custom composable tailored for the project's needs.
6-11: LGTM!The template section of the component is well-structured and follows best practices:
- The div element has classes that control the animation states for open and closed states of the dialog overlay, providing a smooth user experience.
- The
data-dismissable-modalattribute is set totrue, allowing the dialog to be dismissed by clicking on the overlay, which is a common user expectation.- The div element has a high z-index of 1000 to ensure it appears on top of other elements, preventing any visual glitches or unintended interactions.
1-11: Excellent work on the DialogOverlay component!The component is well-implemented, follows best practices, and serves its purpose effectively. Here are some highlights:
- The usage of the
useScrollLockcomposable ensures a good user experience by preventing scrolling of the background content when the dialog is open.- The template section is well-structured, with appropriate classes and attributes for handling animation states and dismissal behavior.
- The component is small, focused, and self-contained, making it highly reusable and maintainable.
Overall, this component can be considered as a good example of a well-designed and implemented UI component. Keep up the great work!
packages/@core/ui-kit/shadcn-ui/src/components/ui/sheet/SheetOverlay.vue (1)
1-11: LGTM!The sheet overlay component is implemented correctly:
- It uses the
useScrollLockcomposable appropriately to lock scrolling when the sheet is open.- The template renders a div with the necessary classes and attributes for animations and dismissal.
- The component follows the expected structure for a Vue SFC.
No changes are needed. Great work!
packages/@core/ui-kit/shadcn-ui/package.json (1)
44-44: Dependency addition looks good, but more information about the package would be helpful.The addition of the
@vben-core/composablesdependency seems reasonable, especially considering theworkspace:*version specification, which suggests a monorepo setup for local development and testing.To better understand the impact of this new dependency on the project, could you please provide more information about the purpose and functionality of the
@vben-core/composablespackage? This will help in assessing how it integrates with and enhances the existing codebase.packages/@core/ui-kit/shadcn-ui/src/components/ui/sheet/SheetContent.vue (4)
15-15: LGTM!The import statement for
SheetOverlayis correct and necessary for using the component later in the code.
19-20: Looks good!The new
modalandopenoptional boolean props are a valid addition to theSheetContentPropsinterface. They will allow the component to better handle modal states and visibility as described in the summary.
33-39: Looks good!The
delegatedPropscomputed property is correctly updated to destructure the newmodalandopenprops along with the existingclassandsideprops. The destructured props are correctly excluded from the returneddelegatedobject.
49-49: Looks good!The conditional rendering of
SheetOverlaybased on theopenandmodalprops is a valid change to improve the component's display logic. It aligns with the objectives described in the summary to enhance the user experience by allowing the component to better handle modal states and visibility.packages/@core/composables/src/use-scroll-lock.ts (1)
11-48: LGTM!The
useScrollLockcomposable function is well-implemented and follows best practices:
- It correctly uses the
useScrollLockcomposable from@vueuse/coreto lock the scroll of the document body.- It adjusts the padding of the document body and fixed layout elements to account for the scrollbar width, ensuring that the layout remains consistent when the scroll is locked.
- It uses
tryOnBeforeMountandtryOnBeforeUnmountlifecycle hooks to apply and remove the scroll lock and padding adjustments at the appropriate times.- It uses
requestAnimationFrameto defer the setting of the transition style of the fixed layout elements in thetryOnBeforeUnmounthook, avoiding any potential layout thrashing.Overall, the function is well-structured, readable, and follows best practices for performance and lifecycle management.
scripts/turbo-run/src/run.ts (2)
26-42: LGTM!The changes to the package selection logic are well-implemented and improve the user experience by:
- Prompting the user to select a package when multiple packages are available.
- Gracefully handling cancellation or invalid selection by exiting the process with a clear message.
- Automatically selecting the package when only one is available, streamlining the process.
The code segment handles all possible scenarios correctly.
44-46: LGTM!The error handling mechanism for the scenario where no package is found is correctly implemented. It:
- Logs a clear error message to inform the user about the issue.
- Exits the process with a failure status, indicating that the operation cannot proceed.
This ensures that the user receives appropriate feedback and the process terminates gracefully in case of an error.
packages/@core/ui-kit/popup-ui/src/drawer/drawer.ts (1)
55-58: LGTM!The addition of the
openAutoFocusproperty to theDrawerPropsinterface is a useful enhancement. It allows developers to control the focus behavior when the drawer is opened, which can improve user experience by directing attention to the drawer's content.The property is appropriately typed as
booleanand marked as optional, ensuring backward compatibility. The comment above the property provides clear documentation of its purpose.packages/effects/layouts/src/basic/tabbar/tabbar.vue (2)
25-25: LGTM!The change to destructure
contentIsMaximizefrom theuseContentMaximizehook is a valid improvement. It provides a more direct reference to the maximized state of the content, which can be used to enhance the UI responsiveness based on the content's state.
76-76: Looks good!Changing the
:screenbinding to usecontentIsMaximizeinstead ofpreferences.sidebar.hiddenis a positive improvement. It ensures that the screen state of the tab bar is directly tied to the maximized state of the content, enhancing the responsiveness and accuracy of the UI based on the content's state.packages/@core/ui-kit/shadcn-ui/src/components/ui/dialog/DialogContent.vue (4)
16-17: LGTM!The import statement for the
DialogOverlaycomponent is correct and necessary for using it in the template.
23-24: LGTM!The addition of the
modalandopenprops to theDialogContentPropsinterface is correct and aligns with the component's enhanced functionality as described in the summary.
33-39: LGTM!The update to the
delegatedPropscomputed property to exclude themodalandopenprops is correct and ensures that these props are not passed down to child components.
55-55: LGTM!The conditional rendering of the
DialogOverlaycomponent based on theopenandmodalprops is correct and aligns with the component's enhanced functionality as described in the summary. The@clickevent binding to emit thecloseevent is also correct.packages/@core/ui-kit/popup-ui/src/drawer/drawer-api.ts (1)
42-42: LGTM!The addition of the
openAutoFocusproperty with a default value offalseis a good enhancement to theDrawerApiclass. It provides more control over the focus behavior when the drawer is opened, allowing developers to customize the user experience based on their specific requirements. The property name is clear and self-explanatory, making it easy to understand its purpose.The change is non-breaking as it introduces a new optional property with a sensible default value, ensuring backward compatibility.
packages/@core/base/design/src/css/global.css (1)
42-43: Verify the impact of the change on user interactions.The removal of the
!pointer-events-autodirective and the introduction of the commented-outpointer-events: auto !important;line suggest a change in how pointer events are handled for the body element. This change may have unintended consequences on user interactions.Please ensure that this change is thoroughly tested to confirm that it doesn't introduce any regressions or unintended behavior. Consider the following:
- Test the change across different browsers and devices to ensure consistent behavior.
- Verify that user interactions with elements within the body are not affected negatively.
- Ensure that the change aligns with the intended interaction model for the application.
If the commented-out line is intended to be used, remove the comments and test the behavior with the
pointer-events: auto !important;directive applied.Run the following script to verify the impact of the change on user interactions:
Verification successful
Verified: Change has minimal impact on codebase
The removal of
!pointer-events-autofrom the body style rules inglobal.cssappears to be an isolated change with minimal impact on the codebase. There's only one other occurrence ofpointer-eventsin the codebase (innprogress.css), which sets it tononefor a specific element.While the change seems intentional, please ensure to:
- Test thoroughly for any global interaction issues, especially on elements that might rely on pointer events propagating through the body.
- Consider whether the commented-out
/* pointer-events: auto !important; */line should be removed or uncommented based on your application's requirements.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the impact of the change on user interactions. # Test 1: Search for usage of `pointer-events` in the codebase. # Expect: Occurrences in relevant components and styles. rg --type-add 'style:*.{css,scss,less,sass}' --type style $'pointer-events' # Test 2: Search for elements within the body that may be affected by the change. # Expect: Relevant elements that rely on pointer events. rg --type-add 'template:*.{html,jsx,tsx,vue}' --type template $'<body.*?>[\s\S]*</body>'Length of output: 332
packages/@core/ui-kit/popup-ui/src/drawer/use-drawer.ts (1)
98-98: LGTM!The change to exclude the
'class'attribute from triggering the complexity warning when aconnectedComponentexists is reasonable and does not introduce any issues.The warning message provides clear guidance to use
useVbenDraweror the API to modify the props of Drawer whenconnectedComponentexists, which helps maintain a clean separation of concerns.packages/@core/ui-kit/popup-ui/src/modal/use-modal.ts (1)
110-110: LGTM!The change to exclude the
'class'attribute from triggering the warning in thecheckPropsfunction looks good. It simplifies the handling of the'class'attribute in the context of connected components while maintaining the original functionality for other attributes.packages/@core/ui-kit/popup-ui/src/drawer/drawer.vue (3)
98-101: LGTM!The
handleFocusOutsidefunction correctly prevents the default behavior and stops the propagation of the event to manage the focus when interacting outside the drawer. This ensures that the focus remains within the drawer component.
105-105: LGTM!Setting the
modalproperty tofalsedisables the modal behavior of the drawer. This change aligns with the PR objective of improving the drawer scrollbar experience by allowing the drawer to be scrollable independently of the underlying content.
115-121: Great job on improving the drawer's accessibility and usability!The addition of the
@close-auto-focus,@focus-outside, and@open-auto-focusevent listeners enhances the drawer component in the following ways:
@close-auto-focusand@focus-outside: These event listeners trigger thehandleFocusOutsidefunction, which manages the focus when interacting outside the drawer. This ensures that the focus remains within the drawer, improving accessibility for keyboard users.
@open-auto-focus: This event listener triggers thehanderOpenAutoFocusfunction, which controls the auto focus behavior when the drawer opens based on theopenAutoFocusproperty. This allows flexibility in managing the initial focus state of the drawer.These changes collectively enhance the drawer's accessibility and usability by providing better control over the focus behavior.
packages/@core/ui-kit/popup-ui/src/modal/modal.vue (5)
126-126: LGTM!Calling
e.stopPropagation()when the modal is not dismissable is the correct approach to prevent unintended event bubbling.
147-147: LGTM!Calling
e.stopPropagation()when the modal or the target element is not dismissable is the correct approach to prevent unintended event bubbling.
151-154: LGTM!The new
handleFocusOutsidefunction is a good addition to manage focus events outside the modal. Preventing the default behavior and stopping event propagation ensures that focus is handled appropriately when the modal is open, improving accessibility and user experience.
158-158: LGTM!Setting
:modal="false"is appropriate to ensure that the modal does not behave as a modal dialog in certain contexts, aligning with the PR objective to improve the modal interaction.
176-183: LGTM!The changes to the
DialogContentcomponent are appropriate:
- Setting
:modal="modal"ensures that the modal behavior is controlled by themodalprop value.- Linking the
@close-auto-focusand@focus-outsideevent listeners to thehandleFocusOutsidefunction ensures that focus management is handled appropriately when the modal is interacted with, aligning with the PR objective to improve accessibility and user experience.packages/@core/ui-kit/layout-ui/src/vben-layout.vue (2)
7-7: LGTM!The import statement for
SCROLL_FIXED_CLASSis correct. Based on the summary, this constant is being introduced to enhance the header's behavior during scrolling.
482-487: LGTM!The class binding logic for the header wrapper has been correctly updated to include the
SCROLL_FIXED_CLASS. This change enhances the visual responsiveness of the header as the user scrolls, ensuring that it adheres to the intended design specifications.
… click failure problems and warnings (vbenjs#4391) * fix: improve the dialog and drawer scrollbar experience, fix internal click failure problems and warnings * chore: remove test code
Description
fixed #4376, #4385
Type of change
Please delete options that are not relevant.
pnpm-lock.yamlunless you introduce a new test example.Checklist
pnpm run docs:devcommand.pnpm test.feat:,fix:,perf:,docs:, orchore:.Summary by CodeRabbit
New Features
getScrollbarWidthutility function for accurate scrollbar dimension calculations.useScrollLockcomposable to manage scroll behavior effectively when modals or overlays are displayed.DrawerApiandDrawerPropswithopenAutoFocusproperty for improved focus management.DialogOverlayandSheetOverlaycomponents for modal overlay functionality, preventing background scrolling.Bug Fixes
Documentation
Chores
@vben-core/composables.