Repository navigation
fix(frontend): keep the filter controls on the screen on a phone - #120
Merged
Merged
Conversation
A control in a submenu beside the Add Filter menu has no room on a phone. React Aria flips a side popover to the other side, but it never moves it along that axis, so the date, number, boolean, and multiselect controls went off the screen and the page scrolled sideways. On a phone, the menu now opens each control in the filter dialog, as it already did for text.
useIsMobile returned false on the first render and read the media query in an effect. So on a phone, ResponsiveDialog mounted with the animation of the desktop dialog, which starts from a blur, and then became a bottom sheet. The sheet animation has no filter target, so the sheet stayed blurred. useIsMobile now reads the media query with useSyncExternalStore.
…ltiselect controls These controls have the fixed width of a submenu. In the bottom sheet on a phone they filled only part of the width. Below the md breakpoint they now fill the sheet.
…hone on its side and on a tablet
… has no room for a submenu From 768 px to about 906 px wide, and on a phone on its side, the date submenu still went off the screen. The menu now shows a submenu only at 64rem wide and 32rem high or more. useMediaQuery reads the query on the first render, and useIsMobile uses it with the md breakpoint of Tailwind in rem.
React Aria sets the max height of a popover to the room on the screen, but the submenu popover let the control overflow it. So on a short window, a tall control such as Scheduled At went below the screen. The submenu popover now scrolls. The height limit of the media query is not necessary any more, so the menu checks only the width.
Owner
Author
|
Open findings from the two review rounds, with the reason each stays open:
Other faults seen, not fixed here:
|
Owner
Author
|
Correction to item 4 of the earlier comment: on a phone and a tablet, a submenu-type item that opens as a dialog shows the arrow icon ( |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
The Add Filter menu opened the date, number, boolean, and multiselect controls in a submenu beside the menu. On a phone there is no room beside the menu, so the controls went off the screen and the page scrolled sideways. React Aria flips a side popover to the other side, but it never moves the popover along that axis, so no collision option can keep a side submenu on the screen. When the screen has no room for a submenu, the menu now opens the control in the filter dialog that the text control already uses. On a phone that dialog is a bottom sheet.
Closes #114
What changed
FilterPropertyMenushows a submenu only at(width >= 64rem). The widest control, date Between, needs about 911 px beside the sidebar. Below that width each control opens inFilterControlDialog.useMediaQueryreads the query on the first render, withuseSyncExternalStore, anduseIsMobileuses it with(width < 48rem), themdbreakpoint of Tailwind. Before,useIsMobilereturnedfalseon the first render. So on a phone eachResponsiveDialogmounted with the blur of the desktop dialog animation, became a sheet, and stayed blurred. The text filter sheet on main has this fault.md.Scope
Blast Radius
useIsMobilealso drivesResponsiveDialogand the sidebar. Both now get the right value on the first render, not one render later. From 768 px to 1023 px, and on a phone on its side, the controls now open in a centered dialog in place of a submenu. At 1024 px and 1280 px every rect matches main.Verification
frontend/tests/features/filter-demo/. On main, the controls failtoBeInViewportat 390x844 (Created Date ratio 0.354), 844x390 (0.737), and 820x1180 (0.815), the five phone sheets haveblur(4px), and at 1280x512 the Scheduled At (Local) control ends at 530.28 px, below the 512 px window. With the fix, 10 pass, and 100 pass with--repeat-each=10.scrollWidth634 before, x=0 to 390 andscrollWidth390 after. At 844x390 and 820x1180,scrollWidthwas 890 before and equals the viewport after. At 1280x800 every submenu rect matches main. At 1280x512 every submenu ends at y=500.pnpm lint,pnpm build,pnpm test(80 passed), andpnpm test:e2e(21 passed).Is Blocked, Status, and the menu at 390 px after the fix:
667x375 before, after, and after with the sheet scrolled to Add Filter: