feat(insights): add % of total option to line, bar and area trend insights (fixes #13728, #11276) #16699

Arun-chaitanya · 2023-07-20T17:39:21Z

Problem

Allow 100% stacked area/bar chart (#13728)

Changes

Without Percent Stack View

With Percent Stack View

Bugs Noticed - Update (FIXED)
I can right away see one bug that if I switch on "Show value on series" at first it is showing percentages as values, but when I hover it is showing their original value.

Screen.Recording.2023-07-20.at.11.06.09.PM.mov

I am working on it. @pauldambra

👉 Stay up-to-date with PostHog coding conventions for a smoother review.

How did you test this code?

pauldambra

looks good so far to me...

@thmsobrmlr is way more familiar than me with insights these days 🥇 tagging him for visibility

frontend/src/queries/nodes/InsightViz/insightDisplayConfigLogic.ts

thmsobrmlr · 2023-07-21T12:02:35Z

Don't have time for a full review at the moment, but here are some fly-by comments:

Does the plugin work for other chart types as well? We've had customer requests for a line / area charts as well e.g. Feature Request - % of total Area chart #11276. I'd like to move forward with a solution that can work for these cases as well.
What do we think about naming? At the moment we have a mix of "percent stack view" and "100% stacked". Maybe something along the lines of "percentage of total"?
More a nit, but the axis is missing the percentage sign (we have an axis display option that would format the axis accordingly).

Arun-chaitanya · 2023-07-21T12:12:17Z

@thmsobrmlr point 1 and 3 can be easily achievable. I will make the required changes and let you know. For point 2, you can suggest me the label name :)

Arun-chaitanya · 2023-07-23T17:42:36Z

Hi @thmsobrmlr , I have done points 1 and 3 as suggested by you and added the percentStack option view for the area and line graph as well.

frontend/src/scenes/insights/views/LineGraph/LineGraph.tsx

pauldambra · 2023-07-26T08:22:24Z

I've not made time to run it yet but from a pass over the code this is looking really good @Arun-chaitanya 🙌

thmsobrmlr

Had a more in-depth look and unfortunately I have quite a bunch of follow up comments code wise and I think I also found two bugs:

When you go to e.g. a funnels insight, then go back to the the trends insight, the option for the percent stack isn't displayed initially and you need to reload the page to see the option
For an insight with a breakdown it doesn't seem to aggregate the values correctly

Hope my comments are clear & you aren't discouraged by them. Let me know if you need clarification or assistance with anything.

frontend/src/lib/constants.tsx

thmsobrmlr · 2023-07-26T13:44:30Z

frontend/src/queries/nodes/InsightViz/utils.ts

@@ -106,6 +106,22 @@ export const getShowValueOnSeries = (query: InsightQueryNode): boolean | undefin
    }
 }

+export const getShowPercentStackView = (query: InsightQueryNode): boolean | undefined => {
+    if (PERCENT_STACK_VIEW_DISPLAY_TYPE.includes(getDisplay(query) as ChartDisplayType)) {


I'd like to split this function up into two parts:

one part that should stay in this function related to getting the show_percent_stack_view from the respective insight filter (this is what other functions in this file do as well)

the other part where we override the value with undefined for chart display types that don't support the percent stack view. this should go in a kea selector with display and showPercentStackView as input and the same output

I think there is also a bug since this only considers the case where the query has a display set, but not the default display.

Also only trends insights support the percent stack view (see below how you implemented formatYAxisLabel)

Resolved: Also only trends insights support the percent stack view (see below how you implemented formatYAxisLabel)
Resolved: I think there is also a bug since this only considers the case where the query has a display set, but not the default display.

About the above two points, I understood the first point, the second point was not that clear to me.

frontend/src/scenes/insights/EditorFilters/percentStackViewLogic.ts

frontend/src/types.ts

frontend/src/scenes/insights/views/LineGraph/LineGraph.tsx

frontend/src/scenes/insights/aggregationAxisFormat.ts

frontend/src/queries/nodes/InsightViz/InsightDisplayConfig.tsx

Arun-chaitanya · 2023-07-26T16:25:25Z

@thmsobrmlr I have resolved most of the comments. And 1st of the 2 bugs is solved. I am not able to reproduce the 2nd bug as I don't have much data in my test env. Is there any way I can reproduce it? Also, can you comment on the above 1 doubt?

Arun-chaitanya · 2023-07-27T12:23:45Z

@thmsobrmlr I was also able to solve the second bug also. Pheew! Now I just have one doubt about your comment, but If I am right I am managing that thing by passing checked={!!showPercentStackView} to Checkbox, so I need not manage undefined case specially. But you can give the correct opinion.

thmsobrmlr

Thank you again for the PR @Arun-chaitanya, everything is looking good now & it's great to finally get this feature in. I'll merge the PR on Monday.

Arun-chaitanya added 2 commits July 20, 2023 22:59

show percentage stack view for bar graph

44e5cfc

add percent stack view for bar graph

ad8baa0

Arun-chaitanya changed the title ~~[DRAFT] - Allow 100% stacked area/bar chart #13728~~ fix - [DRAFT] - Allow 100% stacked area/bar chart #13728 Jul 20, 2023

Arun-chaitanya changed the title ~~fix - [DRAFT] - Allow 100% stacked area/bar chart #13728~~ fix(insights): [DRAFT] - Allow 100% stacked area/bar chart #13728 Jul 20, 2023

Arun-chaitanya and others added 2 commits July 21, 2023 16:39

Merge branch 'PostHog:master' into master

febcb4f

constant datalabel when stacked100 is enabled

d9533ec

Arun-chaitanya mentioned this pull request Jul 21, 2023

Allow 100% stacked area/bar chart #13728

Closed

pauldambra reviewed Jul 21, 2023

View reviewed changes

frontend/src/queries/nodes/InsightViz/insightDisplayConfigLogic.ts Outdated Show resolved Hide resolved

ternary operation instead of if else

b143b61

Arun-chaitanya and others added 3 commits July 23, 2023 19:44

Merge branch 'PostHog:master' into master

20acc84

adding percentStackView to Line and area graph

6d2b738

Y-axis label, hiding unitsBox for percentStackView

9564947

removing from stickiness graph

60a247a

Twixes requested a review from thmsobrmlr July 24, 2023 17:00

pauldambra reviewed Jul 26, 2023

View reviewed changes

frontend/src/scenes/insights/views/LineGraph/LineGraph.tsx Outdated Show resolved Hide resolved

Arun-chaitanya changed the title ~~fix(insights): [DRAFT] - Allow 100% stacked area/bar chart #13728~~ fix(insights): Allow 100% stacked area/bar chart #13728 Jul 26, 2023

thmsobrmlr requested changes Jul 26, 2023

View reviewed changes

PR comment related changes

d60f811

Arun-chaitanya force-pushed the master branch 2 times, most recently from 7471be0 to d60f811 Compare July 26, 2023 16:15

Arun-chaitanya added 2 commits July 26, 2023 21:53

renaming formatYAxisLabel to formatPercentStackAxisValue

7e8a513

renaming to formatPercentStackAxisValue

022abaa

Arun-chaitanya added 3 commits July 26, 2023 22:53

persisting the checkbox value after saving insight

ccee100

passing true false to checkbox

c2e1f08

line graph stacked not working bug solved

ed84b5d

thmsobrmlr changed the title ~~fix(insights): Allow 100% stacked area/bar chart #13728~~ fix(insights): Allow 100% stacked area/bar chart (fixes #13728, #11276) Jul 28, 2023

thmsobrmlr changed the title ~~fix(insights): Allow 100% stacked area/bar chart (fixes #13728, #11276)~~ feat(insights): add % of total option to line, bar and area trend insights (fixes #13728, #11276) Jul 28, 2023

thmsobrmlr added 6 commits July 28, 2023 21:33

review amendmends

214b507

Merge branch 'master' into Arun-chaitanya/master

1cc2889

update schema

3540866

update visual regression tests

650fe0b

update more snapshots

27cfe97

update more snapshots

e000cab

thmsobrmlr self-requested a review July 29, 2023 14:54

thmsobrmlr approved these changes Jul 29, 2023

View reviewed changes

thmsobrmlr merged commit 34f7a9d into PostHog:master Jul 31, 2023

Gilbert09 mentioned this pull request Sep 12, 2023

fix: insights tooltip percentage #17386

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(insights): add % of total option to line, bar and area trend insights (fixes #13728, #11276) #16699

feat(insights): add % of total option to line, bar and area trend insights (fixes #13728, #11276) #16699

Arun-chaitanya commented Jul 20, 2023 •

edited

Loading

pauldambra left a comment

thmsobrmlr commented Jul 21, 2023

Arun-chaitanya commented Jul 21, 2023

Arun-chaitanya commented Jul 23, 2023

pauldambra commented Jul 26, 2023

thmsobrmlr left a comment

thmsobrmlr Jul 26, 2023

thmsobrmlr Jul 26, 2023

Arun-chaitanya Jul 26, 2023

Arun-chaitanya commented Jul 26, 2023

Arun-chaitanya commented Jul 27, 2023 •

edited

Loading

thmsobrmlr left a comment

feat(insights): add % of total option to line, bar and area trend insights (fixes #13728, #11276) #16699

feat(insights): add % of total option to line, bar and area trend insights (fixes #13728, #11276) #16699

Conversation

Arun-chaitanya commented Jul 20, 2023 • edited Loading

Problem

Changes

How did you test this code?

pauldambra left a comment

Choose a reason for hiding this comment

thmsobrmlr commented Jul 21, 2023

Arun-chaitanya commented Jul 21, 2023

Arun-chaitanya commented Jul 23, 2023

pauldambra commented Jul 26, 2023

thmsobrmlr left a comment

Choose a reason for hiding this comment

thmsobrmlr Jul 26, 2023

Choose a reason for hiding this comment

thmsobrmlr Jul 26, 2023

Choose a reason for hiding this comment

Arun-chaitanya Jul 26, 2023

Choose a reason for hiding this comment

Arun-chaitanya commented Jul 26, 2023

Arun-chaitanya commented Jul 27, 2023 • edited Loading

thmsobrmlr left a comment

Choose a reason for hiding this comment

Arun-chaitanya commented Jul 20, 2023 •

edited

Loading

Arun-chaitanya commented Jul 27, 2023 •

edited

Loading