UA: Adding FunnelChartNext #1733

envex · 2024-10-04T16:20:56Z

What does this implement/fix?

Adding FunnelChartNext and SparkFunnelChart.

Eventually FunnelChartNext will replace the current FunnelChart, but for the sake of speed, we're going to support 2 versions.

Figma
Doc

Note

Have a sync with Pablo to finalize x axis labels on smaller screen sizes

Does this close any currently open issues?

Partially resolves https://github.com/Shopify/core-issues/issues/78198

What do the changes look like?

With scaling:

Also adds a SparkFunnelChart for smaller card sizes

Storybook link

https://6062ad4a2d14cd0021539c1b-trqraxynco.chromatic.com/

Before merging

Check your changes on a variety of browsers and devices.
Update the Changelog's Unreleased section with your changes.
Update relevant documentation, tests, and Storybook.
Make sure you're exporting any new shared Components, Types and Utilities from the top level index file of the package

github-actions · 2024-10-04T19:52:14Z

size-limit report 📦

Path	Size	Loading time (3g)	Running time (snapdragon)	Total time
polaris-viz-core-cjs	61.73 KB (0%)	1.3 s (0%)	789 ms (+13.82% 🔺)	2.1 s
polaris-viz-cjs	229.64 KB (+2.42% 🔺)	4.6 s (+2.42% 🔺)	1.8 s (+5.91% 🔺)	6.4 s
polaris-viz-esm	185.82 KB (+2.18% 🔺)	3.8 s (+2.18% 🔺)	1.1 s (-15.94% 🔽)	4.8 s
polaris-viz-css	5.82 KB (+2.22% 🔺)	117 ms (+2.22% 🔺)	362 ms (+81.07% 🔺)	479 ms
polaris-viz-esnext	193.02 KB (+2.25% 🔺)	3.9 s (+2.25% 🔺)	1.3 s (-1.48% 🔽)	5.2 s

midokab · 2024-10-09T22:38:14Z

Looks good to me. A few things:

how do we specify what the summary % is? is it just the bottom of the funnel?
are all the rates calculated in the front end? This is informational because we need to be able to have an open and closed funnel with different session counts that calculate different rates (without needing to introduce different metrics) cc @williecostello
how would the comparisons look?
we should consider renaming the "reached this step" - is that clear enough that its directly related to the last step? @MeredithCastile
not important: but in the example, it looks like the reached next step and dropped off are flipped

envex · 2024-10-10T13:53:19Z

@midokab

how do we specify what the summary % is? is it just the bottom of the funnel?

The main summary is (lastValue / firstValue) * 100.

are all the rates calculated in the front end? This is informational because we need to be able to have an open and closed funnel with different session counts that calculate different rates (without needing to introduce different metrics)

Yes, those are calculated on the front-end, but if the API will give us those values, we can just accept strings/numbers.

how would the comparisons look?

No clue. Any thoughts @pabmtl

not important: but in the example, it looks like the reached next step and dropped off are flipped

Can you post a screenshot of what's flipped?

williecostello · 2024-10-10T14:29:36Z

Are all the rates calculated in the front end? This is informational because we need to be able to have an open and closed funnel with different session counts that calculate different rates (without needing to introduce different metrics)

Yes, those are calculated on the front-end, but if the API will give us those values, we can just accept strings/numbers.

This sounds good to me. Just to clarify: For a visualization like this, the API should only need to provide the count values (i.e., Sessions, Sessions with cart additions, Sessions that reached checkout, Sessions that completed checkout). Currently the plan is to have two sets of these metrics available in the API, one for the open funnel and one for the closed funnel. The API will also include separate metrics for (at least some of) the rates displayed in this viz (e.g., Conversion rate (open funnel), Conversion rate (closed funnel), Added to cart rate (closed funnel), etc.). But the API shouldn't need to pass through any of these rate metrics in order to generate this viz. They should just be consistent with the rates displayed in this viz (which, given everything I've seen about this, they should be).

pabmtl · 2024-10-10T16:03:04Z

The steps label have been updated

Figma

Tooltips are slightly off

The hover interaction for the main columns should only happen on the bar and not the entire section

envex · 2024-10-10T18:08:03Z

@pabmtl I'm not seeing updated steps.

Also, the hover interaction being in the entire section is how the tooltips work on all the other charts, so I don't think we want to deviate from that.

pabmtl · 2024-10-10T18:33:54Z

@envex here's a screenshot

envex · 2024-10-10T18:44:16Z

Ahh, the labels and everything we set via web, so if they're wrong in storybook that's fine, we can change them when we implement in web.

carysmills

Will go through the code soon, but from 🎩

it feels a bit weird when both these tooltips show up, or at least when they overlap

Is it intentional there's no tooltip on the first 100% bar?
Sparkchart isn't showing up for me with just today's data
Can we check accessibility handling? Right now tabbing through the main chart just goes to the connectors. I don't see an aria-label on the spark one
The text gets a bit squishy at small widths and will be worse in some other languages/with other values. Any thoughts?

envex · 2024-10-16T16:07:49Z

it feels a bit weird when both these tooltips show up, or at least when they overlap

Going to look into this. I'm thinking we shouldn't try and use the same Tooltip logic as the other charts and just share the UI.

Is it intentional there's no tooltip on the first 100% bar?

Yes

Sparkchart isn't showing up for me with just today's data
Can we check accessibility handling? Right now tabbing through the main chart just goes to the connectors. I don't see an aria-label on the spark one

I'll dig in

The text gets a bit squishy at small widths and will be worse in some other languages/with other values. Any thoughts?

According to the Figma it should truncate, but we can run it by UX

pabmtl · 2024-10-16T16:11:42Z

The text gets a bit squishy at small widths and will be worse in some other languages/with other values. Any thoughts?

According to the Figma it should truncate, but we can run it by UX

I think updating to the new labels will help with this.

pabmtl · 2024-10-16T16:12:33Z

Those secondary hovers on the transition portion can also be removed, we should keep them as part of the code as a prop if marketing needs to use them and have them show by default with no interactions

envex · 2024-10-16T17:02:26Z

The text gets a bit squishy at small widths and will be worse in some other languages/with other values. Any thoughts?

According to the Figma it should truncate, but we can run it by UX

I think updating to the new labels will help with this.

Those labels will come from the API, we don't do any conversion, so if the labels in the API aren't changing we won't get any new benefit.

pabmtl · 2024-10-16T17:09:49Z

@MeredithCastile mentioned that this was already in progress. I'll circle back with her when she gets back.

…

On Wed, Oct 16, 2024 at 1:02 PM Matt Vickers ***@***.***> wrote: The text gets a bit squishy at small widths and will be worse in some other languages/with other values. Any thoughts? According to the Figma <https://www.figma.com/design/zYBKEXuelQ4eSSkyhuc9s7/%E2%9A%A1%EF%B8%8F-FUNNEL-UPDATES?node-id=717-19863&node-type=frame&t=pi3sMx4AU2ywAJCv-0> it should truncate, but we can run it by UX I think updating to the new labels will help with this. Those labels will come from the API, we don't do any conversion, so if the labels in the API aren't changing we won't get any new benefit. — Reply to this email directly, view it on GitHub <#1733 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AN6Z4NRGZU4E2BDCM2B6Q63Z32L3RAVCNFSM6AAAAABPMGJOOCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIMJXGQYTKMBWGY> . You are receiving this because you were mentioned.Message ID: ***@***.***>

-- *Pablo Boerr* Staff Product Designer [image: Shopify] <https://www.shopify.com/?utm_medium=salessignatures&utm_source=hs_email>

envex · 2024-10-16T17:32:05Z

@pabmtl Awesome!

carysmills

Some initial comments from a first pass, but will need to go over this again later, when you've responded to the first comments.

I didn't leave this comment everywhere, but let's move out the pixel values, colours and constants currently at the top of files into a constants file for this chart.

Related to our chat yesterday: what do you think about making a new directory for this chart (and possibly future others) that's called experimental or something, so we indicate to any other consumers of the repo that the chart and API are unstable? Could also indicate that in the changelog

Can we add any tests that would be helpful?

carysmills · 2024-10-16T17:12:56Z

packages/polaris-viz/src/components/FunnelChartNext/utilities/calculate-dropoff.ts

@@ -0,0 +1,3 @@
+export function calculateDropOff(value: number, nextValue: number) {
+  return ((nextValue - value) / value) * 100;


I wonder if we should calculate this in the viz or if it should be passed in 🤔 I'm fine with keeping it as is for now, but maybe something to consider for a more refined version of the chart. I'm wondering if the dropoff might ever be calculated differently

We talked about getting this from the API but Willie suggested we keep it on the front-end (for now at least).

carysmills · 2024-10-16T17:14:23Z

packages/polaris-viz/src/components/FunnelChartNext/utilities/get-funnel-bar-height.ts

+  rawValue: number,
+  yScale: ScaleLinear<number, number>,
+) {
+  const rawHeight = Math.abs(yScale(rawValue) - yScale(0));


We'll never have a negative bar value, right? I think this might fall apart if we did

It'll "clamp" it to MIN_BAR_HEIGHT if it's negative.

I think in the future we may want to support negative, but for now 🤷

If we end up letting users use this for unexpected data, like the example we've seen, we'll need there to be proper handling for negatives.

packages/polaris-viz/src/components/Labels/SingleTextLine.tsx

carysmills · 2024-10-16T17:17:44Z

packages/polaris-viz/src/components/FunnelChartNext/utilities/get-tooltip-position.ts

+  formatPositionForTooltip: (index: number) => TooltipPosition;
+}
+
+export function getTooltipPosition({


There have been some bugs lately with tooltip positions in notebooks and on the dashboard. Can we make sure we test this with the chart further down/to the left and right on the page?

@envex does this need any rethinking based on recent tooltip changes?

I don't think so because this handles tooltips on it's own, not like the other charts.

Probably good to just spot check that it works fine on mobile and chart further down/to the left and right on the page like you mentioned above.

packages/polaris-viz/src/components/FunnelChartNext/Chart.tsx

carysmills · 2024-10-16T19:38:40Z

packages/polaris-viz/src/components/FunnelChartNext/components/FunnelChartXAxisLabels.tsx

+const LINE_GAP = 5;
+const LINE_PADDING = 10;
+const GROUP_OFFSET = 10;
+const LABEL_FONT_SIZE = 12;


could also move these to a constants file

Yeah, I thought about that but they're only used in this file so moving them felt a little off.

packages/polaris-viz/src/components/FunnelChartNext/components/FunnelChartXAxisLabels.tsx

envex · 2024-12-10T13:03:52Z

@mollerjorge My understanding is that the tooltip percentages are the differences between the numbers in the tooltips, not the total chart numbers.

I don't think we should be showing "Dropped off" section at all on the first bar chart tooltip

For this one, why don't you think we should show the dropoff section in the first tooltip? We don't show it in the last section because there's no dropoff at the last step, but there's a dropoff in the first step.

michaelnesen · 2024-12-10T13:06:31Z

I understand that the <SparkFunnelChart is used for small dashboard cards, but should we make the regular <FunnelChartNext user friendly on smaller devices when it gets displayed in the metric report as well.

@mollerjorge I paired with Pablo on the labels yesterday and just pushed up some changes that should make the labels more friendly on smaller screen sizes (font get's smaller and formatted values are hidden)

mollerjorge · 2024-12-10T13:34:54Z

@mollerjorge My understanding is that the tooltip percentages are the differences between the numbers in the tooltips, not the total chart numbers.

I don't think we should be showing "Dropped off" section at all on the first bar chart tooltip

For this one, why don't you think we should show the dropoff section in the first tooltip? We don't show it in the last section because there's no dropoff at the last step, but there's a dropoff in the first step.

I see now, each bar chart has its own dropoff.

What was a bit confusing to me was that I thought of it like this "ok I'm hovering over the first step in the funnel, why is there a dropoff here if its the first step? I was expecting to read the dropoff on the second bar"

But I guess that's fine because the dropoff comes with each bar.

Regarding the percentages, what does 87,62% mean in the screenshot I sent? 🤔

envex · 2024-12-10T14:16:37Z

@mollerjorge I think (and it's been a while since I originally wrote it) is:

We only calculate the dropoff: ((nextValue - value) / value) * 100; so (47887 - 54654) / 54654 * 100;

So our dropoff percentage is 12.38.

The 87.86% is just 100 - 12.38.

54,654   87.86%
47,887   12.38%

pabmtl · 2024-12-10T15:36:36Z

@michaelnesen the Funnel spark, truncation is too big, should be a lot smaller

.eslintrc

packages/polaris-viz-core/src/constants.ts

packages/polaris-viz/src/components/FunnelChartNext/Chart.tsx

carysmills · 2024-12-10T15:28:28Z

packages/polaris-viz/src/components/FunnelChartNext/Chart.tsx

+          color={PERCENTAGE_COLOR}
+          fontWeight={600}
+          targetWidth={drawableWidth}
+          fontSize={24}


Can we use a const for this and will it be different at different sizes at all?

Will confirm with Pablo 👀

The main percentage font size is the same on different card sizes

packages/polaris-viz/src/components/FunnelChartNext/Chart.tsx

carysmills · 2024-12-10T18:21:34Z

packages/polaris-viz/src/components/FunnelChartNext/components/Tooltip/Tooltip.scss

+}
+
+.Row {
+  font-size: 12px;


Will we want this to be a different size on mobile?

Is this changing?

Based off the Figma the tooltip size looks the same. I think Pablo has some thoughts but he is out. Just looking at some other charts, it doesn't look like font size change in tooltips on mobile, I can make a note about this and revisit with Pablo unless you have any strong opinions.

packages/polaris-viz/src/components/FunnelChartNext/components/Tooltip/Tooltip.tsx

carysmills · 2024-12-10T18:35:13Z

packages/polaris-viz/src/components/FunnelChartNext/utilities/get-tooltip-position.ts

+  formatPositionForTooltip: (index: number) => TooltipPosition;
+}
+
+export function getTooltipPosition({


@envex does this need any rethinking based on recent tooltip changes?

packages/polaris-viz/src/components/shared/FunnelChartSegment/components/ScaledSegment.tsx

michaelnesen · 2024-12-13T22:50:05Z

@mollerjorge

nit: animation seems slightly off on the first barchart. It only animates the dropoff portion

Added an animation for the scaled segment!

<SparkFunnelChart is going to be used in small dashboard cards. I tried adjusting the height and width but it doesn't seem to repond to small heights.

The chart container has a min-height property of 40px for spark charts that comes from the theme. In web, we override this property so it shouldn't be a problem

I wonder if we should add a tooltip on the Sessions icon to explain the scaling effect?

Added a tooltip for the scale icon 👍

packages/polaris-viz/src/components/FunnelChartNext/Chart.tsx

carysmills

Almost there!

On this PR let's:

finish cleaning up the types/names - remove references to xAxisLabels, remove tooltipLabels from the spark chart

Followup:

Add tests for all new components that did not have them added on this PR
Add error states for both charts - as far as I can see that wasn't done

packages/polaris-viz/src/components/FunnelChartNext/Chart.tsx

packages/polaris-viz/src/components/FunnelChartNext/FunnelChartNext.tsx

packages/polaris-viz/src/components/SparkFunnelChart/SparkFunnelChart.tsx

packages/polaris-viz/src/components/FunnelChartNext/components/FunnelChartXAxisLabels.tsx

v1.0.0-funnel-chart-next-beta.1 v1.0.0-funnel-chart-next-beta.2 v1.0.0-funnel-chart-next-beta.3 Export SparkFunnelChart v1.0.0-funnel-chart-next-beta.4 Tweak tooltips

carysmills

🚢

let's make sure we add tests as a followup

I also had a comment back in October about handling negatives. Can you look into whether it will be possible for merchants to get into that state with this chart and if so, what we need to do?

michaelnesen · 2024-12-18T18:21:48Z

@carysmills thanks for the reviews, here's a recap of what's done and the follow ups for future reference:

Recap

In this PR

Added FunnelChartNext and SparkFunnelChart
Added scaling feature to both charts

Follow ups

envex force-pushed the envex/funnel-chart-next branch 2 times, most recently from 8316d3e to 2dc5f46 Compare October 4, 2024 19:49

envex force-pushed the envex/funnel-chart-next branch 3 times, most recently from c72c654 to 7af7534 Compare October 9, 2024 20:11

envex force-pushed the envex/funnel-chart-next branch from 7af7534 to f75c242 Compare October 10, 2024 18:42

carysmills reviewed Oct 16, 2024

View reviewed changes

envex force-pushed the envex/funnel-chart-next branch 2 times, most recently from 53eb2f8 to 3ff9400 Compare October 17, 2024 13:53

michaelnesen force-pushed the envex/funnel-chart-next branch 4 times, most recently from b98270e to 9a2cee4 Compare December 9, 2024 14:24

michaelnesen marked this pull request as ready for review December 9, 2024 14:29

michaelnesen force-pushed the envex/funnel-chart-next branch from 9a2cee4 to 587d4e2 Compare December 9, 2024 14:36

michaelnesen force-pushed the envex/funnel-chart-next branch from 0fdfa56 to 2e92b22 Compare December 10, 2024 13:12

carysmills reviewed Dec 10, 2024

View reviewed changes

michaelnesen force-pushed the envex/funnel-chart-next branch from 3ab1c5a to c76072e Compare December 13, 2024 22:28

michaelnesen force-pushed the envex/funnel-chart-next branch 2 times, most recently from 9e2a5e8 to 57f2180 Compare December 16, 2024 14:14

michaelnesen requested a review from carysmills December 16, 2024 21:19

carysmills reviewed Dec 17, 2024

View reviewed changes

packages/polaris-viz/src/components/FunnelChartNext/Chart.tsx Outdated Show resolved Hide resolved

carysmills reviewed Dec 17, 2024

View reviewed changes

packages/polaris-viz/src/components/FunnelChartNext/Chart.tsx Show resolved Hide resolved

michaelnesen requested a review from carysmills December 17, 2024 18:44

michaelnesen force-pushed the envex/funnel-chart-next branch 2 times, most recently from 26e02c9 to 82dc4b4 Compare December 17, 2024 23:16

carysmills reviewed Dec 18, 2024

View reviewed changes

envex and others added 4 commits December 18, 2024 16:50

Adding FunnelChartNext

6009f28

v1.0.0-funnel-chart-next-beta.1 v1.0.0-funnel-chart-next-beta.2 v1.0.0-funnel-chart-next-beta.3 Export SparkFunnelChart v1.0.0-funnel-chart-next-beta.4 Tweak tooltips

rebase

f3c1e24

Add tabbing through chart accessibility

0df034d

Add scaling feature to funnel chart

fe55d53

michaelnesen force-pushed the envex/funnel-chart-next branch from a482eb8 to fe55d53 Compare December 18, 2024 16:54

carysmills approved these changes Dec 18, 2024

View reviewed changes

michaelnesen mentioned this pull request Dec 18, 2024

[Follow up] Add tests for remaining funnel chart components #1783

Open

michaelnesen approved these changes Dec 18, 2024

View reviewed changes

mollerjorge approved these changes Dec 18, 2024

View reviewed changes

michaelnesen merged commit e4e66fe into main Dec 18, 2024
6 checks passed

shopify-shipit bot temporarily deployed to production December 18, 2024 19:30 Inactive

		@@ -0,0 +1,3 @@
		export function calculateDropOff(value: number, nextValue: number) {
		return ((nextValue - value) / value) * 100;

UA: Adding FunnelChartNext #1733

UA: Adding FunnelChartNext #1733

Conversation

envex commented Oct 4, 2024 • edited by michaelnesen Loading

What does this implement/fix?

Does this close any currently open issues?

What do the changes look like?

Storybook link

Before merging

github-actions bot commented Oct 4, 2024 • edited Loading

size-limit report 📦

midokab commented Oct 9, 2024

envex commented Oct 10, 2024

williecostello commented Oct 10, 2024

pabmtl commented Oct 10, 2024

envex commented Oct 10, 2024

pabmtl commented Oct 10, 2024

envex commented Oct 10, 2024

carysmills left a comment • edited Loading

Choose a reason for hiding this comment

envex commented Oct 16, 2024

pabmtl commented Oct 16, 2024

pabmtl commented Oct 16, 2024

envex commented Oct 16, 2024

pabmtl commented Oct 16, 2024 via email • edited Loading

envex commented Oct 16, 2024

carysmills left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

envex commented Dec 10, 2024

michaelnesen commented Dec 10, 2024

mollerjorge commented Dec 10, 2024 • edited Loading

envex commented Dec 10, 2024 • edited Loading

pabmtl commented Dec 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelnesen commented Dec 13, 2024

carysmills left a comment

Choose a reason for hiding this comment

carysmills left a comment

Choose a reason for hiding this comment

michaelnesen commented Dec 18, 2024 • edited Loading

Recap

In this PR

Follow ups

envex commented Oct 4, 2024 •

edited by michaelnesen

Loading

github-actions bot commented Oct 4, 2024 •

edited

Loading

carysmills left a comment •

edited

Loading

pabmtl commented Oct 16, 2024 via email •

edited

Loading

carysmills left a comment •

edited

Loading

mollerjorge commented Dec 10, 2024 •

edited

Loading

envex commented Dec 10, 2024 •

edited

Loading

michaelnesen commented Dec 18, 2024 •

edited

Loading