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(experiments): better result refresh buttons #27586

Merged
merged 3 commits into from
Jan 17, 2025

Conversation

jurajmajerik
Copy link
Contributor

@jurajmajerik jurajmajerik commented Jan 16, 2025

Changes

Prompted by @andehen: https://posthog.slack.com/archives/C07PXH2GTGV/p1736935461402799?thread_ts=1736935389.854549&cid=C07PXH2GTGV

  • Show last refresh time for the first primary metric with a button to refresh all metrics right away
image

How did you test this code?

👀

@jurajmajerik jurajmajerik requested a review from a team January 16, 2025 10:29
Copy link
Contributor

github-actions bot commented Jan 16, 2025

Size Change: 0 B

Total Size: 1.13 MB

ℹ️ View Unchanged
Filename Size
frontend/dist/toolbar.js 1.13 MB

compressed-size-action

Copy link
Contributor

@andehen andehen left a comment

Choose a reason for hiding this comment

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

Nice! Looks good 👍

type="secondary"
size="xsmall"
onClick={() => {
if (isSecondary) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if it would be better for the user if we refreshed both here as:

  • this is likely what the user wants (it does not make sense to show update results for one but not the other)
  • we currently do reload the whole result component so it kind of looks like both should be updated

Copy link
Contributor Author

@jurajmajerik jurajmajerik Jan 16, 2025

Choose a reason for hiding this comment

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

Good point :) fixed in 1a9b520 (#27586)

we currently do reload the whole result component

Yes but I want to fix this too, in a follow up

Copy link
Contributor

@danielbachhuber danielbachhuber left a comment

Choose a reason for hiding this comment

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

My two cents: I think I prefer the position on the top right of the page. This location is more difficult to find, and it's weird to display it twice when it does the same thing for both.

I like the time indicator, though.

@jurajmajerik
Copy link
Contributor Author

@danielbachhuber agree, makes sense! Moved it to the top bar

image

@danielbachhuber danielbachhuber self-requested a review January 16, 2025 14:31
Copy link
Contributor

@danielbachhuber danielbachhuber left a comment

Choose a reason for hiding this comment

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

🔥

@jurajmajerik jurajmajerik merged commit 5628177 into master Jan 17, 2025
101 checks passed
@jurajmajerik jurajmajerik deleted the experiments-metric-refresh-btn branch January 17, 2025 09:02
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.

3 participants