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

Sessions filters design #2987

Merged
merged 23 commits into from
Jan 19, 2021
Merged

Sessions filters design #2987

merged 23 commits into from
Jan 19, 2021

Conversation

macobo
Copy link
Contributor

@macobo macobo commented Jan 18, 2021

Changes

This follows figma design from https://www.figma.com/file/gQBj9YnNgD8YW4nBwCVLZf/PostHog-App?node-id=1086%3A9814

This UI should (more or less) be ready to be released.

Planning to add a few tests tomorrow but wanted to make review go smoother by putting this out sooner.

Screenshots:
2021-01-18_21-41
2021-01-18_21-42
2021-01-18_21-45
2021-01-18_21-47
2021-01-18_21-47_1
2021-01-18_21-47_2

Commits

  • Rename SessionsTable to SessionsView
  • Filter new menu
  • Add icons to saved filters
  • Show two groups for sessions filters
  • Play with margins
  • Dont display filter box if no filters applied
  • Put search box below dates
  • Support inline filter boxes
  • Add filters button
  • Update stying along with boldness
  • Columns and spacing
  • Show and tag after inputs
  • Allow passing column properties
  • Allow saving sessions filter
  • Add loading skeleton
  • Use feature flags
  • Update descriptions

Checklist

  • All querysets/queries filter by Organization, by Team, and by User
  • Django backend tests
  • Jest frontend tests
  • Cypress end-to-end tests

@timgl timgl temporarily deployed to posthog-new-design-sess-02xeuz January 18, 2021 19:52 Inactive
@PostHog PostHog deleted a comment from ChatBot-2021 Jan 19, 2021
Copy link
Contributor

@paolodamico paolodamico left a comment

Choose a reason for hiding this comment

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

First off, in an overall sense, this looks amazing, loving it 😍! The new filtering thing does take a tad to get used too but then it rocks. Oh and a special mention of the all filters option, seems like a small thing but it's very, very helpful. A few comments, nothing big,

  • The search bar should be full width and I think the placeholder should say "filter sessions by users, actions, events, ..." to maintain the same language of the filters. The margin at the top and bottom of the box also seems a bit off (too short).
  • The buttons on the edit filter drawer should be save on the right and delete on the left, i.e. the default option should be save, not delete. Might even be better to use a link button (keeping the trash icon) instead of a full button.
  • When adding filters from the dotted line add filter button, I think each section that is not the current one should be collapsed, because intuitively if I tap on add filter in the user properties section, I expect to see more user properties filters.
  • Now that I've used it, I'm thinking we need to add a notice when there are filters that haven't been applied. i.e. if you make any changes to filters and you haven't click on "Apply filters", we show a text somewhat prominently (maybe in warning color), maybe below the save filters button? Wdyt? To avoid confusion with users that haven't applied filters.
  • I seem to have found an edge case (see screenshot below). I also managed to get a $is_set property with all 3 inputs. Though can't figure out the reproduction steps, sorry about this.
  • We should limit the width of the filters bar regardless of how long a filter name is to avoid the case below.

Improvements

  • I'm thinking we should have an independent scroll on the filters bar. Seems intuitive as this is a separate component piece (like the nav bar). For context, what lead me to think this is that when you select a preset/custom filter, the position where you land can get you confused. In my case (probably not a real world scenario), sessions load so fast that it looked like the filters didn't work at first. I would also expect to get right unto the sessions list (i.e. somewhat scrolled from the filters) after selecting a filter.
  • Normally I'm against modals, but intuitively I get the feeling that editing a filter should be in a modal not in a drawer, but I don't have hard facts for this. Perhaps it's just because the drawer is so empty (visually not looking so great) that my mind goes to a modal. @corywatilo any feedback here?
  • Maybe add a confirmation to the delete? Definitely don't think is worth soft deleting, but just something like Popconfirm.
  • When the filter editing/adding drawer opens, auto-focus the text input. Also I would change the placeholder text to something like "Name your filter" or even better add a label with "Filter Name" and use the placeholder for examples. Finally, it would be helpful to limit the input's max length, maybe 24 chars or something around that?
  • Maybe it'd be cool to put the filters inside a collapse box? That way the user can collapse the filters while going through the sessions table? Particularly thought of this for when you have a custom filter with a ton of sub-filters that you don't care to see (because your custom filter name already gives you all the info you need).

@@ -82,6 +82,7 @@ function PropertyPaneContents({
onComplete()
setThisFilter(propkey, newValue, newOperator, type)
}}
columnOptions={{ flex: 1 }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like there are some variable re-definitions unrelated to this PR but that might be worth taking the opportunity to address. Didn't update this directly to avoid causing you any merge conflicts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not quite sure what you mean - this particular prop is needed for alignment in sessions filters.

frontend/src/scenes/sessions/filters/AddFilterButton.tsx Outdated Show resolved Hide resolved
frontend/src/scenes/sessions/Sessions.scss Outdated Show resolved Hide resolved
frontend/src/scenes/sessions/filters/EditFiltersPanel.tsx Outdated Show resolved Hide resolved
import { SaveFilter } from 'scenes/sessions/filters/SaveFilter'

const ICONS: Record<SavedFilter['id'], JSX.Element | undefined> = {
all: <UserOutlined />,
Copy link
Contributor

Choose a reason for hiding this comment

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

Not convinced on this icon for the all filter (i.e. no filter), any suggestions @corywatilo or @lottiecoxon ?

@macobo
Copy link
Contributor Author

macobo commented Jan 19, 2021

Thanks for the review @paolodamico.

Implemented some suggestions, creating an issue on the others. I'm treating session filters as throw-away code currently and want to avoid getting too deep into more advanced functionality which will make it hard to throw this away.

Done

The search bar should be full width and I think the placeholder should say "filter sessions by users, actions, events, ..." to maintain the same language of the filters. The margin at the top and bottom of the box also seems a bit off (too short).

We should limit the width of the filters bar regardless of how long a filter name is to avoid the case below.

When the filter editing/adding drawer opens, auto-focus the text input. Also I would change the placeholder text to something like "Name your filter" or even better add a label with "Filter Name" and use the placeholder for examples. Finally, it would be helpful to limit the input's max length, maybe 24 chars or something around that?

Not done

The buttons on the edit filter drawer should be save on the right and delete on the left, i.e. the default option should be save, not delete. Might even be better to use a link button (keeping the trash icon) instead of a full button.

Maybe add a confirmation to the delete? Definitely don't think is worth soft deleting, but just something like Popconfirm.

Normally I'm against modals, but intuitively I get the feeling that editing a filter should be in a modal not in a drawer, but I don't have hard facts for this. Perhaps it's just because the drawer is so empty (visually not looking so great) that my mind goes to a modal. @corywatilo any feedback here?

I copied what feature flags are doing here. Let's create a consistent guideline for this.

I seem to have found an edge case (see screenshot below). I also managed to get a $is_set property with all 3 inputs. Though can't figure out the reproduction steps, sorry about this.

#2981

I'm thinking we should have an independent scroll on the filters bar. Seems intuitive as this is a separate component piece (like the nav bar).

Not quite sure what you mean by this.

Maybe it'd be cool to put the filters inside a collapse box? That way the user can collapse the filters while going through the sessions table? Particularly thought of this for when you have a custom filter with a ton of sub-filters that you don't care to see (because your custom filter name already gives you all the info you need).

Now that I've used it, I'm thinking we need to add a notice when there are filters that haven't been applied. i.e. if you make any changes to filters and you haven't click on "Apply filters", we show a text somewhat prominently (maybe in warning color), maybe below the save filters button? Wdyt? To avoid confusion with users that haven't applied filters.

When adding filters from the dotted line add filter button, I think each section that is not the current one should be collapsed, because intuitively if I tap on add filter in the user properties section, I expect to see more user properties filters.

Agreed, but this is semi-complicated - is it worth doing if this is semi-throwaway as we make filters overall more consistent. I think this can wait until release.

@macobo macobo temporarily deployed to posthog-new-design-sess-02xeuz January 19, 2021 10:03 Inactive
@macobo macobo temporarily deployed to posthog-new-design-sess-02xeuz January 19, 2021 10:36 Inactive
Copy link
Contributor

@paolodamico paolodamico left a comment

Choose a reason for hiding this comment

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

Great stuff @macobo! Please find my comments below. Not opposed to merging as is, but here are my thoughts anyways on what's worth including in this release.

  • Related to the not done stuff. Even if we have the drawer for feature flags, I think it's fine to use a modal here as it's a different use case, but let's table this for a separate PR.
    • Great point on the consistency with feature flags! Though, the feature flags have no delete confirmation because they have soft deletion, but I agree we can work out something for both in a separate PR.
    • The separate scroll I think it's worth doing. Basically it's the same behavior you have in the main navigation bar. If you scroll the whole page (i.e. the content on the page), the navigation bar stays in position. Filters should behave the same way.
  • On the improvements, I would consider doing the last 2.
    • It can definitely be confusing & lead the user to think there's a bug if they forget to click on save filters. Further, as the default behavior everywhere else is to refresh the content automatically when changing filters, I think this is particularly important.
    • It is quite confusing (and makes me think) if I tap add button under the cohorts filters and I see person properties. Detracts from the overall filtering experience.

Agreed on everything else

@paolodamico paolodamico merged commit a612a6b into master Jan 19, 2021
@paolodamico paolodamico deleted the new-design-sessions branch January 19, 2021 14:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants