parcel-plugin-ogimage icon indicating copy to clipboard operation
parcel-plugin-ogimage copied to clipboard

Fix to run through all html files, not just one index.html

Open nothingrandom opened this issue 5 years ago • 7 comments

Fixes my issue, https://github.com/lukechilds/parcel-plugin-ogimage/issues/5

  • loops through all .html files
  • only tries to change the og:image if it exists
  • grabs the important bit of the url (origin), so that you can put the page name into the og:url

nothingrandom avatar Jan 14 '20 13:01 nothingrandom

@nothingrandom thanks for working on this!

Have you tested the absolute URL is correct for files in sub directories?

For example create a structure like:

card.png
index.html
about.html
pages/test.html

And check the resulting absolute og:image value is correct for all pages?

lukechilds avatar Jan 15 '20 09:01 lukechilds

After taking a further look, I noticed you're now using URLogUrlContent.origin. This changes the behaviour and will break any site that isn't using the origin as the site root.

For example imagine if the site root is https://example.com/website and the og:url is also https://example.com/website.

The current code would change og:image in index.html from /card.png to https://example.com/website/card.png.

After merging this PR it would be changed to https://example.com/card.png which would be incorrect.

lukechilds avatar Jan 15 '20 09:01 lukechilds

Have you tested the absolute URL is correct for files in sub directories?

Yes. Partly the purpose for this PR.

After merging this PR it would be changed to https://example.com/card.png which would be incorrect.

I get what you're saying, but it's not necessarily incorrect. For our usecase all the images are in the root directory.

Maybe a better solution would be to optionally set the image root, and if you don't then it resorts to the way you had it originally eg origin/directory/image.ext.

Do you know if I can do something like og:imageroot and it be considered legal for the spec? I can't see anything about creating own meta tags using the og structure as a prefix, so I presume it's okay.

@lukechilds

nothingrandom avatar Jan 15 '20 11:01 nothingrandom

I get what you're saying, but it's not necessarily incorrect. For our usecase all the images are in the root directory.

Right but the root directory of the parcel project isn't necessarily the same as URLogUrlContent.origin.

If you have your parcel project hosted at https://example.com/website then your parcel directory root is hosted at https://example.com/website/.

So if you have ${parcelDir}/card.png this will be hosted at https://example.com/website/card.png. That scenario is handled correctly in master, this PR breaks that.

lukechilds avatar Jan 16 '20 09:01 lukechilds

Maybe a better solution would be to optionally set the image root, and if you don't then it resorts to the way you had it originally eg origin/directory/image.ext.

Do you know if I can do something like og:imageroot and it be considered legal for the spec? I can't see anything about creating own meta tags using the og structure as a prefix, so I presume it's okay.

You don't need to set the image root, you already have all the information you need.

You know the root dir, you know the URL of the current page, and you know the path of the current html file relative to the root dir. That's all the context you need to construct the absolute image url.

lukechilds avatar Jan 16 '20 10:01 lukechilds

Right, should be sorted now?

nothingrandom avatar Jan 16 '20 10:01 nothingrandom

Are you able to test the dir structure posted in https://github.com/lukechilds/parcel-plugin-ogimage/pull/6#issuecomment-574568166 with the parcel root set as both a top level domain and a subdirectory?

And can you post the results here?

I just realised that parcel changes relative urls to absolute. e.g if you have card.png parcel will change it to /card.9190ce93.png. This looks like it'll break on projects hosted in a subdirectory and possibly also pages in a subdirectory in a project that's hosted at a root dir.

lukechilds avatar Jan 23 '20 07:01 lukechilds