Skip to content

feat(funnels): Redesigned funnel viz #9412

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

Merged
merged 21 commits into from
Apr 15, 2022
Merged

feat(funnels): Redesigned funnel viz #9412

merged 21 commits into from
Apr 15, 2022

Conversation

Twixes
Copy link
Member

@Twixes Twixes commented Apr 13, 2022

Problem

Resolves #8159 and resolves #8990.

Changes

Introduces new funnel viz that's much more effective at displaying results. Design by Chris in Figma.
Feature-flagged under lemon-funnel-viz.

To-dos:

  • Add tooltip to bars
  • Hook up persons modal to bars
  • Improve column sizing
  • Remove feature flag lemon-funnel-viz + old FunnelStepTable code
  • Improve color palette
  • Indicate scrollability
  • Fix sizing in InsightCard

Screenshots in comment below.

@Twixes
Copy link
Member Author

Twixes commented Apr 14, 2022

Updated screenshots:

Page-level

Before

before-main

After

after-main

Card

Before

before-card

After

after-card

@Twixes Twixes marked this pull request as ready for review April 14, 2022 11:38
@Twixes
Copy link
Member Author

Twixes commented Apr 14, 2022

This should be reviewable now. I put the new viz behind feature flag lemon-funnel-viz to avoid this PR getting even bigger. The remaining to-dos will be addressed in follow-up PRs.

Copy link
Collaborator

@mariusandra mariusandra left a comment

Choose a reason for hiding this comment

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

Looks like code 👍

Copy link
Contributor

@alexkim205 alexkim205 left a comment

Choose a reason for hiding this comment

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

This is a huge improvement on the current steps table. Some considerations below that aren't blocking. Feel free to tackle in a separate PR as well.

  1. We seem to stop assigning colors past a certain point (try $current_url breakdown).

Screen Shot 2022-04-14 at 9 49 26 AM

  1. Long breakdown names hide rest of table.

Screen Shot 2022-04-14 at 9 52 37 AM

@clarkus
Copy link
Contributor

clarkus commented Apr 14, 2022

image

What happens when the width of the step is larger than the width of the label? I feel like we need to base the sizing here on something consistent across steps. The chart feels broken because of the variable-width steps to me. It also would make it so steps with more verbose names could outcompete or displace adjacent steps.

@Twixes
Copy link
Member Author

Twixes commented Apr 14, 2022

@mariusandra I take it you mean "Looks like good code" or "Looks like fine code"… because technically this is "code" too:

function isEven(number) {
	if (number == 0) return true
	if (number == 1) return false
	if (number == 2) return true
	if (number == 3) return false
	if (number == 4) return true
	if (number == 5) return false
	if (number == 6) return true
	if (number == 7) return false
	if (number == 8) return true
	if (number == 9) return false
	if (number == 10) return true
}

@alexkim205 Thanks! Fixed both.

@clarkus Basically each step's column is as wide as its bars OR as wide as its label, whichever happens to be larger. I initially wanted to do equal width columns, but that's very tricky to pull off with table elements while also accommodating to longer step labels (i.e. not truncating them) and keeping the first axis labels columns well-sized, so I went for the current scheme. As a side effect, this also ensures we never use more space for a single step than necessary (which is useful in insight cards particularly). I see how it can be jarring though. I can keep tinkering with table to see what can be done, but at least it doesn't seem to be a worse situation than what we've got now anyway.

@Twixes
Copy link
Member Author

Twixes commented Apr 15, 2022

I'll merge this for now and keep sizing in mind for a follow-up. @clarkus If anything else is off, let me know.

@@ -20,6 +20,7 @@
border-top-left-radius: $radius;
border-top-right-radius: $radius;
padding-bottom: 5px;
overflow: auto;
Copy link
Contributor

Choose a reason for hiding this comment

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

@Twixes do you remember why this was necessary? Seems to be the cause of the hidden popup

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, I think something was overlaid in a weird way without this, but honestly not sure at the moment. 🤔

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

Successfully merging this pull request may close these issues.

Improve funnels layout for small screens Would like to be able to see all of a funnel in the dashboard view with no horizontal scrollbar
6 participants