fix(island-ui): Filter Dialog - Scroll doesn't work on iOS #15896
Conversation
WalkthroughThe changes to the Changes
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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- libs/island-ui/core/src/lib/Filter/Filter.tsx (6 hunks)
Additional context used
Path-based instructions (1)
libs/island-ui/core/src/lib/Filter/Filter.tsx (1)
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
Additional comments not posted (4)
libs/island-ui/core/src/lib/Filter/Filter.tsx (4)
1-1: LGTM!The import statement change is approved as it is necessary for the new
usePreventBodyScrollhook.
65-88: LGTM!The
usePreventBodyScrollhook is implemented correctly and adheres to the best practices. The code changes are approved.
106-106: LGTM!The
modal: trueparameter is correctly added to theuseDialogStatefunction call to align with the new scroll prevention logic. The code change is approved.
115-115: LGTM!The
usePreventBodyScrollhook is correctly invoked based on the dialog's visibility state and the variant prop. The code change is approved.
Datadog ReportAll test runs ✅ 34 Total Test Services: 0 Failed, 33 Passed Test ServicesThis report shows up to 10 services
🔻 Code Coverage Decreases vs Default Branch (8)
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #15896 +/- ##
=======================================
Coverage 36.93% 36.93%
=======================================
Files 6682 6682
Lines 136504 136503 -1
Branches 38769 38769
=======================================
Hits 50415 50415
+ Misses 86089 86088 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Sentry.
|
RunarVestmann
changed the title
RunarVestmann
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- libs/island-ui/core/src/lib/Filter/Filter.tsx (6 hunks)
Files skipped from review as they are similar to previous changes (1)
- libs/island-ui/core/src/lib/Filter/Filter.tsx
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range, codebase verification and nitpick comments (1)
libs/island-ui/core/src/lib/Filter/Filter.tsx (1)
92-92: Add Unit Tests forusePreventBodyScrollLogicIt appears that the conditional logic in the
usePreventBodyScrollhook is not currently covered by unit tests. To prevent regressions and ensure the intended behavior, consider adding unit tests that specifically test this logic whendialog.visibleis true andvariantis 'dialog'.
- Location to Address:
libs/island-ui/core/src/lib/Filter/Filter.tsxAnalysis chain
Validate the conditional logic in
usePreventBodyScroll.The hook
usePreventBodyScrollis used conditionally based ondialog.visibleandvariant === 'dialog'. This is a smart use of the hook to ensure that scroll prevention only occurs when the dialog is actually visible and in the 'dialog' variant. It would be good to ensure that this logic is covered by unit tests to prevent regressions in behavior, especially in a library component.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify if unit tests cover the conditional logic for usePreventBodyScroll. # Test: Search for unit tests related to usePreventBodyScroll. Expect: Tests that cover the conditional logic. rg --type typescript -A 5 $'usePreventBodyScroll'Length of output: 83
Script:
#!/bin/bash # Description: Verify if unit tests cover the conditional logic for usePreventBodyScroll. # Test: Search for unit tests related to usePreventBodyScroll. Expect: Tests that cover the conditional logic. rg --type ts -A 5 'usePreventBodyScroll'Length of output: 1534
Script:
#!/bin/bash # Description: Search for test files related to the Filter component to check for indirect coverage of usePreventBodyScroll. # Find test files that might be related to the Filter component. fd --type f --name '*Filter*.test.*' # Search for references to the Filter component in test files. rg --type ts 'Filter' --glob '*Filter*.test.*' -A 5Length of output: 490
Script:
#!/bin/bash # Description: Search for test files related to the Filter component to check for indirect coverage of usePreventBodyScroll. # Correct the fd command to find test files related to the Filter component. fd 'Filter.*test.*' # Search for references to the Filter component in test files. rg 'Filter' -A 5 --glob '**/*Filter*.test.*'Length of output: 215
Script:
#!/bin/bash # Description: Perform a broader search for any test files in the repository to check for indirect coverage of usePreventBodyScroll. # Find all test files in the repository. fd '.*test.*' # Search for references to usePreventBodyScroll in all test files. rg 'usePreventBodyScroll' -A 5 --glob '**/*test.*'Length of output: 11846
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- libs/island-ui/core/src/lib/Filter/Filter.tsx (5 hunks)
- libs/island-ui/core/src/lib/Filter/usePreventBodyScroll.ts (1 hunks)
Additional context used
Path-based instructions (2)
libs/island-ui/core/src/lib/Filter/usePreventBodyScroll.ts (1)
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/island-ui/core/src/lib/Filter/Filter.tsx (1)
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
Additional comments not posted (3)
libs/island-ui/core/src/lib/Filter/usePreventBodyScroll.ts (1)
8-34: Good use ofuseEffectand cleanup function.The implementation of
useEffectto manage the scroll behavior based on thepreventBodyScrollflag is correct. The inclusion ofpreventBodyScrollin the dependency array ensures that the effect is properly re-invoked when the flag changes. The cleanup function is well-designed to restore the body's initial state, which is crucial for preventing side effects on other components or hooks.Consider adding more comments to explain why the body's position is set to
fixedand the implications of this change, as it might not be immediately clear to other developers or future maintainers of the code.libs/island-ui/core/src/lib/Filter/Filter.tsx (2)
83-83: Review the use ofmodal: trueinuseDialogState.The addition of
modal: truetouseDialogStateis intended to ensure modal behavior, which is crucial for accessibility and correct dialog management. This change appears to be correctly implemented. However, it would be beneficial to add a comment explaining why this change was necessary, especially since it relates to the scrolling issue addressed in this PR.
178-178: Check the explicit setting ofpreventBodyScroll={false}inDialog.Setting
preventBodyScroll={false}explicitly in theDialogcomponent is a clear and intentional choice to manage scroll behavior. This change is well-documented through the code itself, but adding a comment explaining the reason behind this choice (especially in contrast to the previous behavior) would enhance the clarity and maintainability of the code.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- libs/island-ui/core/src/lib/Filter/usePreventBodyScroll.ts (1 hunks)
Files skipped from review due to trivial changes (1)
- libs/island-ui/core/src/lib/Filter/usePreventBodyScroll.ts
* Fix mobile scroll * Keep track of scroll position * Move hook into separate file * useRef instead of global variable --------- Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
Filter Dialog - Scroll doesn't work on iOS
What
See video:
https://www.icloud.com/photos/#/icloudlinks/0a0egsoTs-s2RKhU64XpLjlmw/0/
On mobile devices (at least on iOS) when you open up the filter dialog, you can't scroll down.
Url's where you can test the filter menu:
After fiddling around with this on my iOS I've managed to fix the issue for me by no longer using the reakit/dialog "prevent scroll" functionality and instead writing my own.
Please help me test this feature on different mobile phone browsers 🙏
Checklist:
Summary by CodeRabbit
Summary by CodeRabbit