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

Does this close any currently open issues?

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

What do the changes look like?

Also adds a SparkFunnelChart for smaller card sizes

Storybook link

https://6062ad4a2d14cd0021539c1b-lugmjeekld.chromatic.com/?path=/story/polaris-viz-charts-funnelchartnext--default

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.42 KB (+0.03% 🔺)	1.3 s (+0.03% 🔺)	1.3 s (+45.18% 🔺)	2.5 s
polaris-viz-cjs	219.01 KB (+1.5% 🔺)	4.4 s (+1.5% 🔺)	2.2 s (+2.02% 🔺)	6.5 s
polaris-viz-esm	179.08 KB (+1.41% 🔺)	3.6 s (+1.41% 🔺)	1.4 s (-5.94% 🔽)	4.9 s
polaris-viz-css	4.87 KB (+3.11% 🔺)	98 ms (+3.11% 🔺)	252 ms (+5.41% 🔺)	349 ms
polaris-viz-esnext	184.92 KB (+1.46% 🔺)	3.7 s (+1.46% 🔺)	1.3 s (-15.78% 🔽)	5 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.

carysmills · 2024-10-16T17:15:36Z

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

 }

 export function SingleTextLine({
  ariaHidden = false,
  color,
  dominantBaseline = 'hanging',
+  fontSize = FONT_SIZE,
+  fontWeight = 400,


We're adding a font weight in the constants, so it feels weird that this is defaulting to a different weight

carysmills · 2024-10-16T17:16:28Z

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

        fontFamily={FONT_FAMILY}
        y={y}
        x={x}
      >
        {truncated}
      </text>
-      <title>{text}</title>
+      {willTruncate && <title>{text}</title>}


Can we check our other uses of this to ensure the changes don't break any existing text?

Before this change it would always display the title so we default to true. There should be no change aside from my new case that I pass false to willTruncate

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?

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

carysmills · 2024-10-16T17:24:38Z

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

+
+  function formatPositionForTooltip(index: number | null): TooltipPosition {
+    // Don't render the tooltip for the first bar
+    if ((index === 0 && xAxisOptions.hide === false) || index == null) {


Why do we need to check xAxisOptions.hide === false? Can't we just check the index?

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 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 added 7 commits October 16, 2024 15:33

Adding FunnelChartNext

aeb69d9

v1.0.0-funnel-chart-next-beta.1

9171802

v1.0.0-funnel-chart-next-beta.2

069055d

Fix truncation and 0 values

0930d86

v1.0.0-funnel-chart-next-beta.3

d7feb66

Export SparkFunnelChart

eef560a

v1.0.0-funnel-chart-next-beta.4

71cbaed

envex force-pushed the envex/funnel-chart-next branch from d5b36f6 to 53eb2f8 Compare October 16, 2024 20:37

Tweak tooltips

3ff9400

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UA: Adding FunnelChartNext #1733

UA: Adding FunnelChartNext #1733

envex commented Oct 4, 2024 •

edited

Loading

github-actions bot commented Oct 4, 2024 •

edited

Loading

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

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

carysmills Oct 16, 2024

envex Oct 16, 2024

carysmills Oct 16, 2024

envex Oct 16, 2024

carysmills Oct 17, 2024

carysmills Oct 16, 2024

carysmills Oct 16, 2024

envex Oct 16, 2024

carysmills Oct 16, 2024

carysmills Oct 16, 2024

carysmills Oct 16, 2024

envex Oct 16, 2024

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

UA: Adding FunnelChartNext #1733

Are you sure you want to change the base?

UA: Adding FunnelChartNext #1733

Conversation

envex commented Oct 4, 2024 • edited 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

envex commented Oct 4, 2024 •

edited

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