-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
fix(stickiness): display correct interval for dates #22304
Conversation
I would like to complete the unit tests with a string input for the first parameter in |
Thanks for taking this on @matzexcom! You can find the |
Thanks, @thmsobrmlr, for the guide. I added the remaining test cases. I have a proposal; I would like to split the function in I would also like to make the first input required for a better type check because I think we don't want the string "undefined" in the front end to be displayed. Is that ok? |
fd934a7
to
5b12868
Compare
Hey @matzexcom, yeah that makes sense to me. Should we get this PR in first and do that in a follow up? (Triggered a CI run right now & will review/merge if that passes) |
…with number input
Head branch was pushed to by a user without write access
de71fa3
to
673837e
Compare
@thmsobrmlr, sure I will create a follow-up PR for the little refactor. Do you know if there is anything I can do here? The failing cypress test earlier today seems unrelated to this PR. I rebased the branch, maybe it will succeed now. |
@matzexcom Let me try to re-run the tests right now. Some of them are flaky and just need to be ran again. |
Just merged this in. Thanks again for the PR! |
--------- Co-authored-by: Matthias Vogel <hub@birdsview.dev>
Datapoint labels on stickiness graphs always count in days (#22209)
Problem
The tooltip does not replicate the correct "group by" interval in the insights stickiness line graph.
Changes
Bugfix: The
getFormattedDate()
method in insightTooltipUtils.tsx had a bug for numeric first param input. Thepluralize()
method had a fixed second argument instead of the interval param.Nit: Changed the name for the first parameter from
dayInput
toinput
, because it can vary between theIntervalType
. (Other possible names could beintervalAmount
ortimeQuantity
)👉 Stay up-to-date with PostHog coding conventions for a smoother review.
Does this work well for both Cloud and self-hosted?
Yes
How did you test this code?
Added automated unit tests for the
getFormattedDate()
method.