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

[META] Integrate with pr-preview #836

Closed
ricea opened this issue Oct 11, 2017 · 30 comments
Closed

[META] Integrate with pr-preview #836

ricea opened this issue Oct 11, 2017 · 30 comments
Labels
editorial Changes that do not affect how the standard is understood.

Comments

@ricea
Copy link
Collaborator

ricea commented Oct 11, 2017

pr-preview would automatically provide diffs of the visible HTML for standard changes. Example output: https://s3.amazonaws.com/pr-preview/whatwg/encoding/ee357f3...6bc288b.html

Unfortunately it doesn't appear to support emu-algify so the integration would be non-trivial. It looks like we'd need a web service to do the translation?

@ricea ricea added editorial Changes that do not affect how the standard is understood. enhancement labels Oct 11, 2017
@domenic
Copy link
Member

domenic commented Oct 12, 2017

I think @tobie has a mode where it can just use the branch-snapshots stuff that already exists, which gets us 90% of the way there. It doesn't help for external contributor PRs (from forks), though.

@tobie
Copy link

tobie commented Oct 12, 2017

What's the issue?

@domenic
Copy link
Member

domenic commented Oct 12, 2017

We run a custom build script beyond just Bikeshed, basically.

@tobie
Copy link

tobie commented Oct 12, 2017

Oh, right. It's node based?

@domenic
Copy link
Member

domenic commented Oct 12, 2017

Yep

@tobie
Copy link

tobie commented Oct 12, 2017

It's post-processing, right? On BS output, not BS source, correct?

@domenic
Copy link
Member

domenic commented Oct 12, 2017

Indeed!

@tobie
Copy link

tobie commented Oct 12, 2017

Give this a spin: https://github.com/tobie/pr-preview#post_processing-optional.

And ping me when you do so I can check the logs. Might completely break, or just work.

@tobie
Copy link

tobie commented Oct 12, 2017

Relevant line of code is here: https://github.com/tobie/pr-preview/blob/master/lib/post-processor.js#L9.

@tobie
Copy link

tobie commented Oct 12, 2017

Does <emu-alg> appear in the output? If so, @dontcallmedom will have to whitelist it in the HTML differ's tidy.

@ricea
Copy link
Collaborator Author

ricea commented Oct 13, 2017

Yes. <emu-alg> is used in the stylesheet so we need it in the output.

@tobie
Copy link

tobie commented Oct 13, 2017

@dontcallmedom already fixed it.

@ricea
Copy link
Collaborator Author

ricea commented Oct 16, 2017

@tobie I tried it approximately 5 minutes ago using the preview feature but it looks like the emu-algify step didn't run.

Pull request containing the config I used is #842.

@tobie
Copy link

tobie commented Oct 16, 2017

Config is pulled from the master branch, not the PR branch.

@tobie
Copy link

tobie commented Oct 16, 2017

Let me go through the logs, though.

@ricea
Copy link
Collaborator Author

ricea commented Oct 16, 2017

Does this mean that testing via https://tobie.github.io/pr-preview/config.html won't work?

@tobie
Copy link

tobie commented Oct 16, 2017

No, it will.

@tobie
Copy link

tobie commented Oct 16, 2017

Tracking this config file issue here: tobie/pr-preview#10

ricea added a commit that referenced this issue Oct 16, 2017
Use the [pr-preview tool](https://tobie.github.io/pr-preview/) to automatically generate previews for pull requests.

Closes #836.
@tobie
Copy link

tobie commented Oct 16, 2017

@ricea just editing the body of any of your existing PRs (just add an extra line break) should trigger pr-preview for it.

@ricea
Copy link
Collaborator Author

ricea commented Oct 16, 2017

@ricea
Copy link
Collaborator Author

ricea commented Oct 16, 2017

Ah, the preview works, just not the diff.

@tobie
Copy link

tobie commented Oct 16, 2017

Yeah, saw that. Tidy still requires custom-element white-listing for now. @dontcallmedom, can you add <emu-xref> to it. @ricea are there others we missed? (we already added <emu-alg>)

@dontcallmedom
Copy link
Contributor

done @tobie

@tobie
Copy link

tobie commented Oct 16, 2017

Thanks a bunch, @dontcallmedom!

@ricea
Copy link
Collaborator Author

ricea commented Oct 16, 2017

<emu-val> is the only other one I found in a quick check.

@tobie
Copy link

tobie commented Oct 16, 2017

Same here.

@tobie
Copy link

tobie commented Oct 16, 2017

Alright, I need to manually wipe the S3 cache, and then we should be good to give it another try.

@tobie
Copy link

tobie commented Oct 16, 2017

Cache wiped. You can give this another go (or alternatively temporarily give me credentials to edit the repo).

@tobie
Copy link

tobie commented Oct 16, 2017

And btw, you don't need to push to the PR to trigger a new update, you can just edit the body of the PR on GitHub's web interface.

@ricea
Copy link
Collaborator Author

ricea commented Oct 16, 2017

Okay, looks good: https://s3.amazonaws.com/pr-preview/whatwg/streams/20f9c64...d054b80.html#ts-intro

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
editorial Changes that do not affect how the standard is understood.
Development

No branches or pull requests

4 participants