Replace hand-rolled dropdown on the dashboard and menu-related UI fixes #5037
Conversation
|
apata
changed the title
There was a problem hiding this comment.
Looks good. I had some comments about further improvements that could be made but this PR already does a lot. Not suggesting any of this needs to be part of this PR.
If the button isn't blurred immediately, as this PR does, user would have to tab off the button for the clear all filters keybind to work.
Yeah. OTOH it breaks the expected behaviour that I can:
- Open dropdown
- Close via escape key
- Open again via enter
I think the ideal behaviour would be that stopPropagation only runs when the popover is open. The event would be allowed to propagate if popover is closed without blurring the button. In that case both escape and enter would work as expected after closing the dropdown. I tried using the open render prop to achieve this but the sequence of events didn't allow me to achieve it (the open prop is already false when the key event handler runs).
Not a blocker for sure, it's a tiny detail.
| @@ -1,28 +1,6 @@ | |||
| /* @format */ | |||
| /* stylelint-disable media-feature-range-notation */ | |||
| /* stylelint-disable selector-class-pattern */ | |||
| .flatpickr-calendar::before, | |||
| : 'md:left-auto md:w-56' | ||
| )} | ||
| > | ||
| <Popover.Panel |
There was a problem hiding this comment.
Nit: The fact that dropdownState changes immediately when the menu is closed sort of negates the close transition. The menu disappears immediately and only the shadow is transitioned away which looks weird if you really pay attention.
Ideally the dropdownState would change to closed after the transition is finished. Or maybe it would be easier to make the different states their own separate Popover.Panel components but I don't know how that would affect the Menu -> Calendar transition.
There was a problem hiding this comment.
Well spotted! I thought it was barely noticeable and was just happy to have finally gotten the following transitions working at all with keyboard and clicks.
- Closed -> Menu
- Menu -> Closed
- Menu -> Calendar
- Calendar -> Closed
- Closed -> Calendar
Will look into it once more: I actually didn't try swapping out the panel as you suggest OR driving Transition and Panel directly with the state.
There was a problem hiding this comment.
Yeah, it works great as is, just a very minor nitpick. Don't spend too much time polishing it :)
| @@ -147,8 +151,12 @@ export function IntervalPicker({ onIntervalUpdate }) { | |||
| enterTo="opacity-100 scale-100" | |||
There was a problem hiding this comment.
Can we use the same consolidated transition props here as the popovers to make sure they're consistent?
…es (#5037) * Try popover * Pass targetRef instead of target, stop Escape clearing filters * Stop Escape clearing filters when popover menus active * Attempt get rid of hand-rolled dropdown * Fix issue with comparison calendar * Almost works * Unify styles * Focus modals on mount * Refactor date picker logic * Replace navigate keybinds with straightforward keybinds * Remove extraneous Calendar component, better props * Attempt optimise menu re-renders * Memoise QueryPeriodsMenu, refactor getDatePeriodGroups * Refactor ComparisonMenu to Popover * Refactor QueryPeriodMenu to Popover * Pull calendar out of components * Refactor calendar to receive position from JS * Simpler calendar API * Add click outside listener for Calendar, fix FiltersBar measurements * Give back top bar room * Update tests * Apply unified button text style * Close calendar on keyboard nav to othe menus as well * Kinda works * Works even better * Working well * Sometimes menu stays active * WIP * Adapter * Works with error * Fixed the error * Remove handrolled isOpen * Introduce shared adapter * Share adapter
* Remove the second line for filters * Remove extraneous class and prevent wrap * Fix jumpyness * Replace hand-rolled dropdown on the dashboard and menu-related UI fixes (#5037) * Try popover * Pass targetRef instead of target, stop Escape clearing filters * Stop Escape clearing filters when popover menus active * Attempt get rid of hand-rolled dropdown * Fix issue with comparison calendar * Almost works * Unify styles * Focus modals on mount * Refactor date picker logic * Replace navigate keybinds with straightforward keybinds * Remove extraneous Calendar component, better props * Attempt optimise menu re-renders * Memoise QueryPeriodsMenu, refactor getDatePeriodGroups * Refactor ComparisonMenu to Popover * Refactor QueryPeriodMenu to Popover * Pull calendar out of components * Refactor calendar to receive position from JS * Simpler calendar API * Add click outside listener for Calendar, fix FiltersBar measurements * Give back top bar room * Update tests * Apply unified button text style * Close calendar on keyboard nav to othe menus as well * Kinda works * Works even better * Working well * Sometimes menu stays active * WIP * Adapter * Works with error * Fixed the error * Remove handrolled isOpen * Introduce shared adapter * Share adapter * Allow dropshadows in a slightly better way, add more comments * Share transition with interval picker menu * Calendar transitions independently * Calendars are in separate popovers * Hide outline on supposedly hidden buttons * Fix arrow appearing on Safari * Fix difference between calendar dropdown and menu dropdown
Changes
Removes handrolled dropdowns and replaces them with headlessui Popovers: https://headlessui.com/v1/react/popover
Also tried to use headlessui Menu component for datepicker, but unfortunately those were too opinionated to work well with our keybinds.
Had to refactor the datepicker: visually that means extremely minor changes (first group doesn't have top padding)
Fixes the issue with calendars positioning off screen, aligning their right edge to right of dropdown or right of visible page on smaller screens.
Fixes issues with Dashboard modals not receiving focus
Fixes issues with Escape clearing filters instead of closing opened menu / popover
Fixes issues with multiple dropdowns being openable at the same time
Optimises top bar components re-rendering too many times for no reason
Tests
Changelog
Documentation
Dark mode