tobie / pr-preview

Adds preview and diff to spec pull requests.
Apache License 2.0
33 stars 16 forks source link

Support for relative URLs #144

Open jandrieu opened 1 year ago

jandrieu commented 1 year ago

Currently, images (and other assets) referenced by relative URLs fail to load properly, making embedded images broken links in the preview.

I believe that if we set the base to point to the PR's raw files, this problem goes away.

<base href="https://raw.githubusercontent.com/[user]/[repo]/[commit]/">

Where the user, repo, and commit point to that particular PR (which I got by inspecting the URLs in the files tab of the PR page).

If I manually add this base in the source, it works. Of course, it's crazy making to put details about the PR into the content of the fork itself (you have to create the PR, then update the fork), but it proved that a good <base> element could solve the problem.

It may be that this is ALWAYS the right approach (base SHOULD be the PR), but at a minimum, I'd like a way to turn it on in a config file if it isn't a good default for some reason.

Can anyone point me to the code where this might be handled? Happy to try a PR, but I'm new to the code base.

I note that it also worked as

<base href="https://raw.githubusercontent.com/[user]/[repo]/[branch]">

When using the incoming fork's user and repo. As a hack, that makes it a bit easier for the PR creator as they can just refer to the right branch in their fork, without needing to know about any particular PR. Unfortunately, it would mean that the PR preview is always looking at the current branch, which may not be correct historically. You could view an old, closed PR, and get the wrong view of the asset. Linking to the PR's commit doesn't have that problem.

This seems like it would be a generally useful improvement.

Thoughts?

tobie commented 1 year ago

Unfortunately, that would break all in-page anchors, no?

jandrieu commented 1 year ago

Ah-hah!

Great catch. I didn't realize that an in-page anchors uses base in that manner. I would have expected the link to not trigger a query back to the server. Clearly the spec says otherwise .

However, it works if you include the filename in the base:

<base href="https://raw.githubusercontent.com/[user]/[repo]/[commit]/[filename]">

You can see this at this PR: https://github.com/w3c/vc-use-cases/pull/147

So, I think this new base will work for everything EXCEPT where the page authors need to manually set base. So, I think if we automatically set it in cases where there is no base, it could work.

jandrieu commented 1 year ago

Btw, we are also using ReSpec data-includes in that file, and they were totally broken in the preview. With the base hack, they work.

We also have in-page anchors that had been broken with the previous base. Now, with the filename, its working for in-page anchors too.

tobie commented 1 year ago

One way around this would be to make this opt-in.

jandrieu commented 1 year ago

Where in the code would we make this change?

tobie commented 1 year ago

So this could be either handled at the PR Preview level:

or as an option on ReSpec or Bikeshed that you can just pass a value to, much like we already pass the URL of the PR, for example.

jandrieu commented 10 months ago

@tobie Anything I can do to help move this forward?

I like the option of adding a config parameter, then injecting the base as appropriate by automatically extracting the PR's URL for that file.

FWIW, passing the URL as a parameter is also a reasonable feature, but would not be much of an improvement over my current hack of manually looking up that URL and injecting the BASE in the file directly. If you have to edit the config file to set the URL, then you have to adjust the URL uniquely for every PR (which is essentially the same amount of work).

If you can send me to the right place in the code to try out this feature, I'll take a look.