Skip to content
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

feat(trends): Add "Decimal places" as a Trends viz option #19330

Merged
merged 8 commits into from
Dec 15, 2023

Conversation

Twixes
Copy link
Member

@Twixes Twixes commented Dec 14, 2023

Problem

Solves ZEN-8274.

Changes

Added a new visualization option (frontend-only) to Trends called "Decimal places":

Screenshot 2023-12-14 at 14 27 15

How did you test this code?

Updated some frontend tests. Let me know if there are any other tests I should add.

Copy link
Contributor

github-actions bot commented Dec 14, 2023

Size Change: +153 kB (+8%) 🔍

Total Size: 1.99 MB

Filename Size Change
frontend/dist/toolbar.js 1.99 MB +153 kB (+8%) 🔍

compressed-size-action

Copy link
Contributor

@thmsobrmlr thmsobrmlr left a comment

Choose a reason for hiding this comment

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

Super cool and works really well. There's a backend side filters_to_query method, where the filter needs to be added as well.

@@ -254,6 +254,7 @@ export const filtersToQueryNode = (filters: Partial<FilterType>): InsightQueryNo
aggregation_axis_format: filters.aggregation_axis_format,
aggregation_axis_prefix: filters.aggregation_axis_prefix,
aggregation_axis_postfix: filters.aggregation_axis_postfix,
decimal_places: filters.decimal_places,
Copy link
Contributor

Choose a reason for hiding this comment

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

To make things extra complicated we now have a backend-side filter_to_query function as well 😉 And yup, let's get rid of filters in the new year.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, updated this too!

const reportChange = useDebouncedCallback(() => {
posthog.capture('decimal places changed', {
decimal_places: trendsFilter?.decimal_places,
})
Copy link
Contributor

Choose a reason for hiding this comment

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

Expanding on the conversation with @benjackwhite #19323 (comment), should I see this as a +1 to remove the eventUsageLogic?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I agree with Ben. I don't think eventUsageLogic is either helpful nor unhelpful… it's just there as boilerplate, but then I don't see a point in using it. It's rare that we want to capture the same event at two call sites, and for reference purposes searching posthog.capture( is just as easy as opening eventUsageLogic.ts.

@Twixes
Copy link
Member Author

Twixes commented Dec 15, 2023

Thanks for taking a look @thmsobrmlr. All conversion and tests should be updated now.
Also I added one small quality of life improvement: "Decimal places" should now only show up in Trends insights that might have fractional values (so e.g. "average property values", but not "total count").

@thmsobrmlr
Copy link
Contributor

Thanks for taking a look @thmsobrmlr. All conversion and tests should be updated now. Also I added one small quality of life improvement: "Decimal places" should now only show up in Trends insights that might have fractional values (so e.g. "average property values", but not "total count").

Makes sense and looks good to me!

@Twixes Twixes merged commit 37d1954 into master Dec 15, 2023
78 checks passed
@Twixes Twixes deleted the var-insight-precision branch December 15, 2023 20:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants