-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Conversation
This should be reviewable now. I put the new viz behind feature flag |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like code 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
@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 |
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; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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. 🤔
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:
lemon-funnel-viz
+ oldFunnelStepTable
codeInsightCard
Screenshots in comment below.