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

Feat: Super Pill and Range Picker #4422

Open
wants to merge 89 commits into
base: main
Choose a base branch
from

Conversation

briangregoryholmes
Copy link
Contributor

@briangregoryholmes briangregoryholmes commented Mar 26, 2024

Fixes #4679
PRD: https://www.notion.so/rilldata/Time-Super-pill-b26f581630a14680b05fb5f2cd2d41e6?pvs=4

This PR incorporates a variety of updates related to the new Time Super Pill. It started as a refactor of the calendar picker, the original description of which can be seen below.

This implementation has been put together with the Time Control store refactor in mind, but sticks to the existing state, pulling only a few helpers or types from the new time controls file. As such, a few things that have just been copied verbatim into new locations will be simplified when integrated with the new store.

To Do:

  • Add custom range selection to comparison (needs feedback)
  • Resolve UI issues with narrower screens (feedback welcome)
  • Ensure implementation matches Figma as close as possible
  • Move subrange to Zoom button
  • Consider solutions to button shifting issue (based on changing range strings)
  • Update tests
  • Add comparison selector to Time Series view
  • Resolve inclusive/exclusive date display issues

Out of Scope:

  • Add zoom functionality from PRD (likely out of scope for this PR)

This PR is a feature-complete, but WIP proposal (initially started with #3824) that both updates the TimeRangeSelector and TimeComparisonSelector menus to the new ShadCN components and makes a related and technically necessary change to our date picker implementation. Additionally, there is some simplification being done in preparation for the refactor of the time control state management.

Looking for experience passes around the new UX, technical review (mostly around date handling), design input and general feedback.

  • Removes the use of Litepicker
  • Adds custom Month and Calendar components
  • Removes the use of domain-less text fields as a secondary form of input
  • Adds Select components from ShadCN
  • Uses dropdown UI elements for month, day and year selection
  • Adds the ability to nudge the selection by different time grains
  • Fixes a critical bug that would extend the selected range by one day
  • Explicitly labels the start and end dates as inclusive/exclusive

Tagging @mindspank and @jkhwu

Screenshot 2024-03-26 at 1 02 59 AM

Closes: #3808

@briangregoryholmes briangregoryholmes self-assigned this Mar 26, 2024
@ericpgreen2
Copy link
Contributor

As mentioned in check-in, first @mindspank will review the UX. After his green light, one of @AdityaHegde / @djbarnwal / myself will do the code review.

@briangregoryholmes briangregoryholmes added the blocker A release blocker issue that should be resolved before a new release label Apr 16, 2024
@ericpgreen2
Copy link
Contributor

Code looks good. I see @jkhwu just finalized the Time "Superpill" PRD, so I think her UX feedback will be important.

@jkhwu
Copy link
Contributor

jkhwu commented Apr 19, 2024

Thanks for your patience, here are my thoughts:
Screenshot 2024-04-19 at 7 37 04 PM

  1. Those little step buttons at the bottom felt a bit buggy, and they may not be necessary. Sometimes (esp after changing the dates manually), those arrow buttons only changed one of the dates, and once when I tried stepping by year it didn't work for either date. Sometimes the year stepper lets you get to a year much further in the future than the dropdown selectors allow. I'd prefer omitting those buttons for now.

  2. When using the calendar layout to click on dates, I found myself frequently entering this state (where the start date was after the end date). Could we simply disallow this from ever happening so I don't trigger a bunch of errors by clicking dates? I propose that instead of entering this state, we simply assume that the earlier selected date is always the start date. Would that be possible?
    Screenshot 2024-04-19 at 7 47 16 PM

  3. Could we set the extended calendar panel to not move around? When trying to advance the months using the arrows, I kept closing the panel by accident because the arrow moves around depending on how many days are in the month. Would be nice to keep those arrows fixed in place.
    Apr-19-2024 19-55-22

Just a note in response to Eric's comment that this component will remain available even after we implement the time super-pill concept, so would be great to spend the time polishing its usability.

@jkhwu
Copy link
Contributor

jkhwu commented Apr 19, 2024

Screenshot 2024-04-19 at 8 15 46 PM
4. Another thought: this menu is getting quite long especially for people viewing on laptops. We could probably move the bottom content into the extended calendar picker pane, which would shorten the menu and also hide the apply button unless it's actually necessary.

@briangregoryholmes briangregoryholmes removed the blocker A release blocker issue that should be resolved before a new release label Apr 24, 2024
@briangregoryholmes briangregoryholmes changed the title Refactor: TimeRangeSelector and Date Picker Feat: Super Pill and Range Picker Apr 26, 2024
@briangregoryholmes briangregoryholmes removed the request for review from ericpgreen2 July 8, 2024 22:21
@jkhwu
Copy link
Contributor

jkhwu commented Jul 12, 2024

Just reviewed, this looks good to me!

@ericpgreen2
Copy link
Contributor

ericpgreen2 commented Jul 15, 2024

I love the new all-or-nothing leaderboard comparison columns!

UXQA –

  1. I navigate from "Last 7 days" comparing "Previous week" to "Last 14 days". The compared range has updated to show the previous 14 days / previous period, but the label still says "Previous week".

    image image
  2. On "Last 12 months", I initially see ["Previous period", "Previous Year"] are available options. After selecting "Previous period", "Previous year" disappears as an option.

    In other cases, if I have this right, it looks like we don't show "Previous period" when we have already have an equivalent "Previous X". E.g. when the time range is "Last 24 Hours", we have "Previous day", and we don't show "Previous period". So, it seems like we should do the same for "Last 12 months" and "Previous year".

  3. Dashboards without a time series probably shouldn't have the first row of the Filters section.

    image
  4. The Custom Time Range picker isn't very keyboard accessible. It doesn't need to be perfect for this first pass, but I most noticed it when I pressed "tab" to try to move from the Start Date to End Date, the dialog closed on me, and the selection wasn't applied.

  5. In the Time Dimension Detail, it's confusing that I can enter a state where I compare a non-time dimension (reflected in the chart) and compare the time dimension (not reflected in the chart). In this case, because the time comparison isn't reflected in the chart, it feels like it should not be possible to enter that mode.

    image
  6. (Existing behavior) I noticed that Pivot comparisons don't work when a Time dimension is included. Maybe we could be explicit about this and toggle off the Comparison in the Super Pill.

    image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feat(explore): Time super pill Dashboard: Date picker is hard to use on small screens
6 participants