Skip to content
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

Replace second line for filters with UI from segments variant D #5022

Merged
merged 11 commits into from
Feb 6, 2025

Conversation

apata
Copy link
Contributor

@apata apata commented Jan 27, 2025

Changes

  • Removes the second line for filters that's shown when saved_segments feature flag is true, replacing it with the single line version from segments variant D and the multicolumned filter menu from the same.
    • For users that don't have the feature flag (pretty much all of them), no visual changes are expected, except a few tiny tweaks that deal with what happens in the top bar in the overflow situation.
  • Refactors filter pills so their drop shadows wouldn't be cut off by overflow: hidden.
  • Creates non-interactive filter pill and pill list versions (in anticipation of segment modals).

Tests

  • Automated tests have been added

Changelog

  • This PR does not make a user-facing change

Documentation

  • This change does not need a documentation update

Dark mode

  • The UI has been tested both in dark and light mode

Visuals

pr-5022 review plausible io_plausible io_f=is,page,__dashboard f=is,country,US f=is,region,US-IL l=US,United%20States l=US-IL,Illinois (1)

@apata apata changed the title Replaces Replaces second line for filters with UI from segments variant D Jan 27, 2025
@apata apata changed the title Replaces second line for filters with UI from segments variant D Replace second line for filters with UI from segments variant D Jan 27, 2025
@apata apata force-pushed the saved-segments/singular-bar branch from a0317c6 to 07e1127 Compare January 27, 2025 19:15
@apata apata added the preview label Jan 28, 2025
Copy link

Preview environment👷🏼‍♀️🏗️
PR-5022

@apata apata requested a review from macobo January 28, 2025 08:18
@apata apata changed the base branch from saved-segments/expand-segments-query-from to master January 28, 2025 09:14
@apata apata force-pushed the saved-segments/singular-bar branch from 07e1127 to 85878b7 Compare January 28, 2025 09:18
@apata apata requested review from ukutaht and removed request for macobo January 28, 2025 09:48
assets/js/dashboard/nav-menu/filter-pill.tsx Outdated Show resolved Hide resolved
ref={seeMoreRef}
dropdownContainerProps={{
['title']: opened ? 'Show less' : 'Show more',
['aria-controls']: 'more-filters-menu',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already have headlessui as a dependency. Consider using the dropdown menu component over a hand-rolled dropdown to get things like aria properties for free.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed with the principle overall. Alas, headlessui doesn't align with accessibility standards (tab navigation between items) and that's why I refactored datepicker out of it last year. I wanted to keep new UI aligned with datepicker, thus I kept this custom component.

Maybe we want to align with the storybook dropdown instead?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

headlessui doesn't align with accessibility standards (tab navigation between items)

What standard specifically? AFAIK accessible menus should be navigable via arrows and items should have tabindex="-1" which headlessUI does https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/menu_role

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, MDN is clear on that! My gut feel had solidified to a standard since the last time we talked about this: #4461 (comment)

Still, the question remains which components to refactor and when. There's a few on the dashboard, but also https://plausible.io/storybook/dropdown is on tab navigation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This turned out to be a bit more involved than I hoped, but I think I managed to get some good results. I've prepped an additional PR against this branch:
#5037

@ukutaht
Copy link
Contributor

ukutaht commented Jan 28, 2025

Problem: when a filter is added the page jumps vertically by a little bit, but enough to be annoying:

Cap.2025-01-28.at.12.17.07.mp4

@apata apata force-pushed the saved-segments/singular-bar branch from 5cc073b to f3332cf Compare January 30, 2025 17:05
@ukutaht
Copy link
Contributor

ukutaht commented Feb 4, 2025

Approved assuming #5037 gets merged

apata added 11 commits February 5, 2025 17:20
…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
@apata apata force-pushed the saved-segments/singular-bar branch from ba6f837 to b7257e4 Compare February 5, 2025 15:20
@apata apata added this pull request to the merge queue Feb 6, 2025
Merged via the queue into master with commit 18ca439 Feb 6, 2025
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants