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

Remove the old images folder #24

Merged
merged 2 commits into from
Jun 2, 2021
Merged

Remove the old images folder #24

merged 2 commits into from
Jun 2, 2021

Conversation

ThomasThelen
Copy link
Member

@ThomasThelen ThomasThelen commented May 27, 2021

The assets/images folder wasn't deleted when I moved it to content/. The is the PR for removing it.

To test:

  1. Checkout
  2. Navigate around the site to make sure the images are still being displayed

Fixes #23

@ThomasThelen ThomasThelen requested a review from robyngit May 27, 2021 20:00
@ThomasThelen ThomasThelen changed the title Remove the old images folder [Draft] Remove the old images folder May 27, 2021
@ThomasThelen ThomasThelen changed the title [Draft] Remove the old images folder Remove the old images folder May 27, 2021
Copy link
Member

@robyngit robyngit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good and works on my end!

This might be outside of the scope of this PR, but when reviewing it, I had some ideas on making it easier for content editors given this new strategy of storing images in /content/images:

  1. Open question: In the markdown front-matter, should we require the /images/ part of the path to the image? If images that content editors add will always be in that directory, isn't it redundant? An alternative idea could be to:

    • in the markdown, specify image paths starting without the /images/ part, so bg_image: "/images/methodology/banner.jpg" becomes bg_image: "methodology/banner.jpg"
    • specify the images directory as a param in the config file (e.g. imageDir: images)
    • then where images are inserted in templates, set the src with something like: {{ path.Join site.Params.imageDir .Params.bg_image }}
  2. Now that we're not dependent on the file structure in the images directory matching the file structure of the markdown/content directory, I think that the images directory structure should be re-organized. Content editors might have an easier time finding and organizing the images they want to use with a simpler directory structure and more specific image names. For example, instead of storing the banner image used on the "Supporters" page like so: /images/about/supporters/banner.jpg, I suggest something like: /images/bannerImages/whale.jpg. This strategy has the following advantages:

    • from the markdown file, it's clear to editors that they are using a banner image of a whale (whereas the file name banner.jpg is unclear)
    • it's a lot easier to change which banner image is used on which page, rather than moving identically named images between different directories.
    • it's easier for editors to browse the banner images that are already available/in use on their website
    • if the content directory structure changes, or a content directory is renamed, the directories in the images directory (and associated image paths) do not need to be updated.
    • the same image can be used on multiple pages without confusion and without duplicating the image file

    I propose following this strategy for all types of images (not just banner images): categorize images by a general type (e.g. banner images, charts, maps, etc.), create a directory for each category, give each image file a name that represents the content of the image.

@ThomasThelen
Copy link
Member Author

ThomasThelen commented May 29, 2021

  1. I think I'd prefer to keep the image/ path because
    a. It's extra obvious where the file is
    b. It avoids any complicated machinery where someone in the future can't figure out why their image isn't being found, only to find a piece of code that's appending things to the path they define. I like the idea of removing redundancy, but also like the idea of keeping things fairly obvious and simple.

I could be wrong about those ^^ though and I'm fine making that change if you want it. b might be a strawman because they could just look at all the other image paths and see that they don't have image/ in the path.

  1. I was hoping that the image directory structure I had would be easy to use. If you're editing a particular goal, then all of the related images are going to be in that goal's image folder. Under the proposed structure, now a user has to care about n folders. If they want to add a map to a particular page, they have to go to the map folder; to add a different banner they have to go to the banner folder-opposed to everything being together in a single place. If a user wants to change an image on page X, they have to hunt the image down instead of just going to page X's image folder and having a clear view of all the content on that page. It also avoids any sort of classification problems: do I create a new folder for this image? It's not a banner and it's not an infographic, so where do I put it? The current way completely avoids any sort of cognition needed for that because the answer is always put it in the page's image directory. In my head the instructions are fairly clear:
  • Create a new md file in the right content directory
  • Create a new folder in images/content/xyz
  • Add the banner file to the directory, name it banner.jpg
  • Any other images for that page should be added to that folder

from the markdown file, it's clear to editors that they are using a banner image of a whale (whereas the file name banner.jpg is unclear)

I would hope that someone can figure out that all of the images named banner.xyz are the banner images on each page. Every page has a banner.xyz and it's always going to be the banner for that page.

it's a lot easier to change which banner image is used on which page, rather than moving identically named images between different directories.

It is. But there's only going to be a single banner for every page. I don't think it would be difficult for someone to just copy + replace an image in a directory (probably a rare event). They wouldn't even need to change the frontmatter if they keep the filename :)

it's easier for editors to browse the banner images that are already available/in use on their website
I agree with this one

if the content directory structure changes, or a content directory is renamed, the directories in the images directory (and associated image paths) do not need to be updated.

It would be a pain to have to go back and rename the image folders, but I don't think that this is going to happen that often. We have a pretty good idea of what the site's structure is like, so I don't see this happening that often in the future

the same image can be used on multiple pages without confusion and without duplicating the image file

I think that this is the biggest weakness in the folder structure. It would be weird (and against the pattern) to include images from other page directories and my way does't have a solution to this other than
a. Completely breaking the pattern and including images from other directories
b. Duplicating images

To me, the last point could be the nail in the coffin if we're going to share images across pages since neither are really acceptable; I'd be fine with the layout that you're proposing.

@ThomasThelen
Copy link
Member Author

Is it actually okay of we merge this? Doing part 2 would require rewriting the front matter and dealing with all of the card images. I'd rather do this after I've finished getting rid of the card images in #22 and then all we'll have are the banners. See #27 as a placeholder

@robyngit robyngit merged commit 59014dd into main Jun 2, 2021
@ThomasThelen ThomasThelen deleted the remove_images_folder branch June 9, 2021 22:04
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

Successfully merging this pull request may close these issues.

Remove static/images
2 participants