refactor(insightTooltipUtils): Splits getFormattedDate to new getFormattedTimeInterval method
"Problem"
In insightTooltipUtils.tsx, the method getFormattedDate may be overloaded. The method generates a formatted date, as the name implies, but it also generates an interval amount like "2 days, 4 weeks, ..." depending on the type of the first argument, which may not be intuitive for future maintainers.
Propose
Extract the part for the interval amount from the method getFormattedDate into a new method getFormattedTimeInterval
Changes
- Created a new method `getFormattedTimeInterval(intervalAmount: number, interval: IntervalType = 'day')``
- Changed Input type from
getFormattedDateto string and make it required - Changed the
InsightTooltipProps.datetostring | numberto reflect the possibility that the date can either be a string (hopefully a date string) or a number - Changed the
getTooltipTitlemethod a bit to reflect the change that the date can be undefined or a number, which would result in a "bad" title - Adds unit tests for
getFormattedTimeIntervalandgetTooltipTitle
Does this work well for both Cloud and self-hosted?
Yes
How did you test this code?
Add a new insight on a dashboard, test the different tabs, and check if the tooltip is the same as before.
@thmsobrmlr I proposed a refactor, which we discussed in #22304, and I created this separate PR. My refactor clarifies the purpose of the insightTooltipUtil methods.
I had to adjust some types because there are cases where the date variable is a number, that was missed in the typing. I changed the nearest type definition because changing the definition of days in the TrendResult interface would result in a big refactor.
I hope my changes don't add more complexity and reduce tech dept for the future.
Thanks for the catch; I thought I had checked that; my mistake. I wrote an e2e test to reflect my introduced bug 🙈 and fixed it. I am not very happy with the nested ternary. I don't know if it is allowed in the coding convention or if I should write a "helper" method for the InsightTooltip Component to get the title.
I am not very happy with the nested ternary. I don't know if it is allowed in the coding convention or if I should write a "helper" method for the InsightTooltip Component to get the title.
The nested ternary is ok, we have many of those in the codebase.
@thmsobrmlr Yes not related to my changes here.
Since commit 31f97c11 the tooltip in stickiness is broken again
the interval type in seriesData?.[0]?.filter?.interval is undefined, the filter property is an empty object {}
I have to comment out/delete this two lines
# "hogql-in-insight-serialization",
# "hogql-insights-preview",
in posthog/settings/feature_flags.py after that the filter property is filled again and the tooltip is shown correctly
@thmsobrmlr Any idea where I can investigate the issue? I tried to find my way in the backend and found out that the filter parameter is built within this method _query_to_filter in posthog/hogql_queries/insights/trends/trends_query_runner.py (maybe)
I tried to start the django server in debug mode but I can not start it and get weird ModuleNotFoundError: No module named 'errors'
This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the stale label – otherwise this will be closed in another week.
This PR was closed due to lack of activity. Feel free to reopen if it's still relevant.