-
Notifications
You must be signed in to change notification settings - Fork 373
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
Load examples manifest via HTTP #4391
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall, but I don't understand why there are two links in the new PR description template
.github/pull_request_template.md
Outdated
* [ ] I have tested the web demo (if applicable): | ||
* [Nightly](https://app.rerun.io/pr/{{ pr.number }}/index.html?manifest_url=https://app.rerun.io/version/nightly/examples_manifest.json) | ||
* [PR](https://app.rerun.io/pr/{{ pr.number }}/index.html) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be elaborated on. As it reads now I would assume the first link would test the nightly version of the viewer, which makes little sense.
Both are using the viewer from this PR afaict, but the first one links to example rrds from the "nightly" build, and the second one links to examples from… what exactly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to add some more information here
@@ -221,15 +291,15 @@ impl ExamplePage { | |||
}); | |||
}); | |||
|
|||
self.examples.iter().for_each(|example| { | |||
for example in examples { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 - it's a bit surprising that clippy::needless_for_each
doesn't catch this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what I changed, but at some point while I was working on this, the lint finally popped up which prompted me to change this
I pulled in an unrelated change, prettier v3 expects lowercase |
What
examples_manifest.json
as part of the binary in favor of downloading it fromapp.rerun.io
re_viewer/build.rs
logic to a separate binary cratemanifest_url
)examples_manifest.json
together with every build of the web app for:/commit
/pr
/version/nightly
/version/{tag}
/prerelease
/latest
Checklist