Skip to content

Add a walkthrough of an actual graph example to explain content #30013

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

Merged
merged 7 commits into from
Jun 24, 2025

Conversation

estherk15
Copy link
Contributor

@estherk15 estherk15 commented Jun 18, 2025

What does this PR do? What is the motivation?

Merge instructions

Merge readiness:

  • Ready for merge

@estherk15 estherk15 requested a review from a team as a code owner June 18, 2025 16:43
@estherk15 estherk15 added the WORK IN PROGRESS No review needed, it's a wip ;) label Jun 18, 2025
@github-actions github-actions bot added Images Images are added/removed with this PR Guide Content impacting a guide labels Jun 18, 2025
Copy link
Contributor

github-actions bot commented Jun 18, 2025

✅ Documentation Team Review

The documentation team has approved this pull request. Thank you for your contribution!

Copy link
Contributor

@estherk15 estherk15 added the editorial review Waiting on a more in-depth review label Jun 18, 2025
@estherk15 estherk15 requested a review from edanaher June 18, 2025 17:00
Copy link
Contributor

@cswatt cswatt left a comment

Choose a reason for hiding this comment

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

Sorry for this annoying review! I started out understanding it and began to get confused—I think I need to see an equation for the computations here.

@cswatt
Copy link
Contributor

cswatt commented Jun 21, 2025

Forgot this note about the graphics. Let's clone/edit the notebook and then take new screenshots:

  • it's really not obvious what each colored line represents, and the displayed key is useless. Let's add aliases to the lines, so each graphic has an informative key that says the blue line is 5-min rollup, the purple line is 30-min rollup, etc.
  • These are not fractions—they're percentages! Let's replace "Fraction" in each graph title with "Percentage"
  • For the last two graphs, it's kind of hard to tell the color difference between the blues. I know it's the classic color palette, but it doesn't make much sense for two groups of two lines, where we're trying to emphasize a difference in mobile vs. total. Maybe we can keep the blue/purple for mobile, and then do a red/yellow for total.

Copy link
Contributor

@edanaher edanaher left a comment

Choose a reason for hiding this comment

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

Overall, I think this looks pretty good, but I had a few questions/comments.

  - Removed sectionf on unique vs. distinct
  - Removed paragraph and images on distinct mobile users
  - Added formulat for distinct user calculations
@estherk15 estherk15 force-pushed the esther/docs-update-rollup-guide branch from 5d97890 to c7d26e5 Compare June 23, 2025 20:19
@estherk15 estherk15 requested a review from cswatt June 24, 2025 16:20
@estherk15 estherk15 removed the WORK IN PROGRESS No review needed, it's a wip ;) label Jun 24, 2025
@estherk15 estherk15 requested a review from edanaher June 24, 2025 18:57
@estherk15 estherk15 merged commit 47cc0b3 into master Jun 24, 2025
25 of 26 checks passed
@estherk15 estherk15 deleted the esther/docs-update-rollup-guide branch June 24, 2025 19:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
editorial review Waiting on a more in-depth review Guide Content impacting a guide Images Images are added/removed with this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants