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

docs: fix example in window.md #8611

Merged
merged 5 commits into from
Dec 19, 2022
Merged

docs: fix example in window.md #8611

merged 5 commits into from
Dec 19, 2022

Conversation

PBI-David
Copy link
Contributor

I think this fixes the following although I can't test on my machine to be certain: #8610

Also minor wording update.

I think this fixes although I can't test on my machine to be certain: #8610

Also minor wording updates.
@domoritz domoritz changed the title Docs: Update window.md docs: Update window.md Dec 19, 2022

### Percent of Total

You can use the window transform to compute an aggregate and attach it to all records. In this case, you can use the [join aggregate](joinaggregate.html) instead of the window transform. Please refer to the [join aggregate examples](joinaggregate.html#examples).

Here we use the window transform to derive the global sum so that we can calculate percentage.

<div class="vl-example" data-name="window_percent_of_total"></div>
<div class="vl-example" data-name="joinaggregate_percent_of_total"></div>
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the fix. We already have a separate page for join aggregate at https://vega.github.io/vega-lite/docs/joinaggregate.html so I don't think we should have this example here. Can you fund the correct example, add it back, or update the text?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, you're right. I think the original example was removed to be replaced with a simpler joinaggregate here: #4554

Maybe we should just delete the section altogether now it is covered by joinaggregate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have updated to remove some redundancy and simplify the text.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we leave things up to line 124 with the link but delete the rest.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, what you did looks great.

@domoritz
Copy link
Member

Can you run prettier?

@PBI-David
Copy link
Contributor Author

Can you run prettier?

Unfortunately not as I don't have an environment set up. I'm editing on the web. I can't see why prettier would fail as it looks clean and is a simple line of text with not much to actually format.

@domoritz domoritz enabled auto-merge (squash) December 19, 2022 20:23
@domoritz domoritz changed the title docs: Update window.md docs: fix example in window.md Dec 19, 2022
@domoritz domoritz merged commit 6c5d266 into vega:next Dec 19, 2022
@domoritz
Copy link
Member

Thank you

@vega-org-bot
Copy link
Collaborator

🚀 PR was released in v5.7.0-next.0 🚀

BradyJ27 pushed a commit to BradyJ27/vega-lite that referenced this pull request Oct 19, 2023
* Update window.md

I think this fixes although I can't test on my machine to be certain: vega#8610

Also minor wording updates.

* Update window.md

Removed redundant example

* Update window.md

* Update site/docs/transform/window.md

* style: prettier

Co-authored-by: Dominik Moritz <domoritz@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants