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

Support for relative URLs #144

Open
jandrieu opened this issue Jul 17, 2023 · 7 comments
Open

Support for relative URLs #144

jandrieu opened this issue Jul 17, 2023 · 7 comments

Comments

@jandrieu
Copy link

jandrieu commented Jul 17, 2023

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
Copy link
Owner

tobie commented Jul 18, 2023

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

@jandrieu
Copy link
Author

jandrieu commented Jul 18, 2023

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: w3c/vc-use-cases#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
Copy link
Author

jandrieu commented Jul 18, 2023

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
Copy link
Owner

tobie commented Jul 18, 2023

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

@jandrieu
Copy link
Author

Where in the code would we make this change?

@tobie
Copy link
Owner

tobie commented Jul 20, 2023

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

  • a new opt-in option in the config file
  • documentation in the README
  • Adding a base element to the generated output (probably just searching for the head element and appending a base element to its first tag).

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
Copy link
Author

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants