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

fix(insights): Improve load more for breakdowns #24231

Merged
merged 22 commits into from
Aug 14, 2024

Conversation

webjunkie
Copy link
Contributor

@webjunkie webjunkie commented Aug 7, 2024

Problem

It was only working if you selected "display other", because presumably you otherwise couldn't tell if more breakdown values are available.

Fixes #23554

Changes

  • make it show up in all cases
  • adjust the wording to have the actual values - since you also modify the query with this and it's persisted when you save
  • use separate hasMore flag that is derived from "other" breakdowns being available. For this to work we ignore the hide_other flag and always calculate them, but hide the label+value later.
image

How did you test this code?

Tested locally

It was only working if you selected "display other"
presumably this was because of the filter conversion?
@webjunkie webjunkie requested review from thmsobrmlr and a team August 7, 2024 08:58
@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

62 snapshot changes in total. 0 added, 62 modified, 0 deleted:

Triggered by this commit.

👉 Review this PR's diff of snapshots.

Copy link
Contributor

github-actions bot commented Aug 7, 2024

Size Change: +34 B (0%)

Total Size: 1.07 MB

ℹ️ View Unchanged
Filename Size Change
frontend/dist/toolbar.js 1.07 MB +34 B (0%)

compressed-size-action

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

2 snapshot changes in total. 0 added, 2 modified, 0 deleted:

  • chromium: 0 added, 2 modified, 0 deleted (diff for shard 1)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

3 snapshot changes in total. 0 added, 3 modified, 0 deleted:

  • chromium: 0 added, 3 modified, 0 deleted (diff for shard 1)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

3 snapshot changes in total. 0 added, 3 modified, 0 deleted:

  • chromium: 0 added, 3 modified, 0 deleted (diff for shard 1)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

# Conflicts:
#	frontend/__snapshots__/scenes-app-insights--trends-area-breakdown-edit--dark--webkit.png
#	frontend/__snapshots__/scenes-app-insights--trends-area-breakdown-edit--dark.png
#	frontend/__snapshots__/scenes-app-insights--trends-area-breakdown-edit--light--webkit.png
#	frontend/__snapshots__/scenes-app-insights--trends-area-breakdown-edit--light.png
#	frontend/__snapshots__/scenes-app-insights--trends-bar-breakdown-edit--dark--webkit.png
#	frontend/__snapshots__/scenes-app-insights--trends-bar-breakdown-edit--dark.png
#	frontend/__snapshots__/scenes-app-insights--trends-bar-breakdown-edit--light--webkit.png
#	frontend/__snapshots__/scenes-app-insights--trends-bar-breakdown-edit--light.png
#	frontend/__snapshots__/scenes-app-insights--trends-line-breakdown-edit--dark--webkit.png
#	frontend/__snapshots__/scenes-app-insights--trends-line-breakdown-edit--dark.png
#	frontend/__snapshots__/scenes-app-insights--trends-line-breakdown-edit--light--webkit.png
#	frontend/__snapshots__/scenes-app-insights--trends-line-breakdown-edit--light.png
#	frontend/__snapshots__/scenes-app-insights--trends-pie-breakdown-edit--dark--webkit.png
#	frontend/__snapshots__/scenes-app-insights--trends-pie-breakdown-edit--dark.png
#	frontend/__snapshots__/scenes-app-insights--trends-pie-breakdown-edit--light--webkit.png
#	frontend/__snapshots__/scenes-app-insights--trends-pie-breakdown-edit--light.png
#	frontend/__snapshots__/scenes-app-insights--trends-table-breakdown-edit--dark--webkit.png
#	frontend/__snapshots__/scenes-app-insights--trends-table-breakdown-edit--dark.png
#	frontend/__snapshots__/scenes-app-insights--trends-table-breakdown-edit--light--webkit.png
#	frontend/__snapshots__/scenes-app-insights--trends-table-breakdown-edit--light.png
#	frontend/__snapshots__/scenes-app-insights--trends-value-breakdown-edit--dark--webkit.png
#	frontend/__snapshots__/scenes-app-insights--trends-value-breakdown-edit--dark.png
#	frontend/__snapshots__/scenes-app-insights--trends-value-breakdown-edit--light--webkit.png
#	frontend/__snapshots__/scenes-app-insights--trends-value-breakdown-edit--light.png
Derive from breakdown other option, which we always query for and then
just discard later on.
@webjunkie webjunkie requested a review from thmsobrmlr August 12, 2024 09:48
@webjunkie
Copy link
Contributor Author

@thmsobrmlr I've added a relatively quick approach/workaround to tell if there are actually more values. Can you check again? Also for experiments, I checked one and now the load more isn't there. But maybe that depends on certain conditions, so would be great if you can also check that.

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

62 snapshot changes in total. 0 added, 62 modified, 0 deleted:

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

2 snapshot changes in total. 0 added, 2 modified, 0 deleted:

  • chromium: 0 added, 2 modified, 0 deleted (diff for shard 1)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

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.

Thanks for the follow-up @webjunkie! Works great now.

Did a couple of stylistic changes, hope you don't mind.

There's only one thing blocking this from merging now: The button only appears on a new insight, but not after saving. I'm assuming this has something to do with the insight serializer not returning the hasMore or similar.

@thmsobrmlr
Copy link
Contributor

Also for experiments, I checked one and now the load more isn't there. But maybe that depends on certain conditions, so would be great if you can also check that.

This was just an artifact of the previous implementation where we would always display the button regardless if there are more values. Experiments only have two values and as such it's working there now.

@webjunkie
Copy link
Contributor Author

with the insight serializer not returning the hasMore or similar.

Okay fixed this now. The hasMore is next to the results... which means it needs to go into the serializer as well. This whole area is a bit cumbersome. I wonder why no other insights had specific fields that needed to be included. We have hasMore also for other query responses, but it seems it's mostly actors responses that are not going to be the result of a saved insight 🤔

@thmsobrmlr
Copy link
Contributor

with the insight serializer not returning the hasMore or similar.

Okay fixed this now. The hasMore is next to the results... which means it needs to go into the serializer as well. This whole area is a bit cumbersome. I wonder why no other insights had specific fields that needed to be included. We have hasMore also for other query responses, but it seems it's mostly actors responses that are not going to be the result of a saved insight 🤔

Yeah it's quite annoying. We did have other insights with problems here and probably still have some edge cases that are unhandled..

@webjunkie webjunkie enabled auto-merge (squash) August 14, 2024 09:18
@webjunkie webjunkie merged commit cea6ac6 into master Aug 14, 2024
93 checks passed
@webjunkie webjunkie deleted the fix/breakdown-limit-load-more branch August 14, 2024 09:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants