search

getgrav / grav-plugin-sitemap

Grav Sitemap Plugin
https://getgrav.org
MIT License
42 stars 42 forks source link

Add support for images #81

Closed JvanderCeelen closed 3 years ago

JvanderCeelen commented 4 years ago

Changed the placeholder for image to plural, in our case we needed multiple images on a page to show up in the sitemaps. With some help in the grav discord I created the url for the image: /user/pages//. This worked for our images. Not sure if this goes for everyone though. You need to use...

sitemap:
    images:
        image-name:
            image-property:

...in the frontmatter. Not sure if this is clear for everybody. Should I adjust the readme to point this out? I'm not a backender, so not really up to snuff with best practices. Please let me know if I need to adjust something.

hughbris commented 4 years ago

This is a useful addition. I made some minor comments against lines of code, plus:

  1. It's worth mentioning in your PR description that you have removed the image property of the sitemap entry object. It seems you are right to remove it, since it does not appear to be used.
  2. This plugin discovers pages automatically. Additions can be listed in the plugin config. There are two methods for exclusions, the page's frontmatter or the plugin's ignore property. I wonder if images could work in a similar fashion, with the option to exclude images is available in images' YAML metafiles. (Perhaps for images, they should be excluded by default with an option to include??). This is more coding, but removes the scenario where a developer of a site with many significant images (to be included in the sitemap) needs to enumerate them all in sitemap.yaml. Not sure I explained that very well
  3. Please update the plugin's README file to show the additional functionality.
JvanderCeelen commented 4 years ago

hey Hugh, thanks for the comments. I will start changing stuff when I have the time for it. Hopefully somewhere this week.

JvanderCeelen commented 4 years ago

Hi Hugh, I made some minor changes to the code. On your first point, I haven't removed the image property from the sitemap entry object, but renamed it to images, plural. I mentioned it in the description. Is that satisfactory?

On your second point, I think I get what you mean, but I'm not sure if it's a good idea. The way I thought about this, is to pick up every image defined in the frontmatter of a page and put them in the sitemap. But I'm not sure people would want that. It could be an equal amount of trouble to get all image which aren't important removed from the sitemap by hand as putting the important ones in. I'm not a SEO guy, so I don't know if it's commen practice to add images in the sitemap or not. Do you have any idea? I could ask our external SEO guy, but I'll wait with that until I have an answer from you.

On your third point, I will update the readme as the added functionality is now and adjust it if we change someting afterwards.

JvanderCeelen commented 4 years ago

Added readme and license tag.

rhukster commented 3 years ago

Due to conflicts, i had to merge this in manually.. Also made some minor changes. Thanks.