Skip to content
This repository has been archived by the owner on Dec 27, 2022. It is now read-only.

Restore changes to parse arrays of images #118

Closed
wants to merge 13 commits into from

Conversation

denisgoryaynov
Copy link
Contributor

It seems that the changes form my previous pull request were overwritten by this commit that implemented compatibility with Gatsby Cloud. This pull request is just to restore the files that allowed parsing arrays of images and query them like this

{
  allStrapiArticle {
    edges {
      node {
        id
        singleImage {
         publicURL
        }
        multipleImages {
          localFile {
            publicURL
          }
        }
      }
    }
  }
}

@alexandrebodin
Copy link
Contributor

This wasn't overriden but moved higher in the file. I don't get what your PR is supposed to add

@denisgoryaynov
Copy link
Contributor Author

denisgoryaynov commented Apr 3, 2020

@alexandrebodin hi, this PR allows to use the publicURL field on arrays of images. Right now if I do something like this:

query Test {
 allStrapiProject {
    nodes {
      title
      multipleMedia {
        localFile {
          publicURL
        }
      }
      section {
        media {
          localFile {
            publicURL
          }
        }
      }
    }
  }
}

gatsby fails to build because there are no nodes for the images. What this PR does is the same that my last PR did, #90 it allows to query the localFile field when using the Multiple Media type in the strapi admin panel.

Edit: the reason why I opened this PR is that if you check the commit that I linked you can see that it deleted the changes I made to src/normalize.js from my previous pull request

Edit 2: while my solution does the job PR #112 is better and solves the need for using the localFile field, it only has a minor issue but I already commented to let the author know. If that issue gets fixed I hope that the PR gets merged because this is a critical issue for my team and the others that had my same problem.

@alexandrebodin
Copy link
Contributor

Ok, Thank you. I'll check this. The diff is kinda hard to read so will have to test this thouroughly :)

@alexandrebodin
Copy link
Contributor

@denisgoryaynov Can you fix the conflicts in yout PR ?

@denisgoryaynov
Copy link
Contributor Author

@alexandrebodin yes, I can do it later today

@dsfrederic
Copy link

@denisgoryaynov is it possible to use Gatsby-image in combination with multiple files?

@carlaiau
Copy link

@denisgoryaynov how is the conflict resolve going? Keep up the great work!

@denisgoryaynov
Copy link
Contributor Author

@alexandrebodin I fixed the conflicts, sorry if it took me so long

src/normalize.js Outdated
}
// If we have cached media data and it wasn't modified, reuse
// previously created file node to not try to redownload
if (cacheMediaData && item.updatedAt === cacheMediaData.updatedAt) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, this should be an option as the timestamps are configurable per model. (e.g by default on mongo it is camelcased createdAt / updatedAt and on sql databases it would be created_at / updated_at.)

Copy link
Contributor Author

@denisgoryaynov denisgoryaynov Apr 15, 2020

Choose a reason for hiding this comment

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

Oh I didn't know that, I just changed it because at some point in the code the variable was set as updatedAt but then inside that if the value being used was updated_at which was always undefined for me and I just though that it was an inconsistency. I can try to test it and fix it tomorrow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alexandrebodin I fixed it with your suggestion but I have a question.
What I did is to create a new variable like this: const itemUpdatedAt = item.updatedAt || item.updated_at, is there a better way to check what database is being used or this method should be fine?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi, This should be fine for now. We will need to fetch model metadata to be more dynamic in the future though :) Will test this weekend se we can release next week ;)

@alexandrebodin
Copy link
Contributor

HI @denisgoryaynov can you just fix the DCO check on this PR plz ? :D

Copy link
Contributor

@alexandrebodin alexandrebodin left a comment

Choose a reason for hiding this comment

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

Can you explain why you use localFile as a key here ? Can't we just have an array of images ?

denisgoryaynov and others added 4 commits April 20, 2020 13:24
Signed-off-by: denisgoryaynov <denis.gor27@gmail.com>
Signed-off-by: denisgoryaynov <denis.gor27@gmail.com>
Signed-off-by: Alexandre Bodin <bodin.alex@gmail.com>
Signed-off-by: denisgoryaynov <denis.gor27@gmail.com>
Signed-off-by: denisgoryaynov <denis.gor27@gmail.com>
@denisgoryaynov
Copy link
Contributor Author

@alexandrebodin I fixed all the issues. I used the localFile key because at the time I didn't know how to do it and that's what other plugins were using (for example gatsby-source-wordpress) and unfortunately I didn't really have much time to take a better look at it and see if it was possible to solve the issue without using the localFile key.

@carlaiau
Copy link

Got ya branch working for a current project I'm working on using gatsby-image. Hope it gets pulled in, thanks for the great work!

@peterfortuin
Copy link

I have also check out this branch in my current project and it solves my problem with arrays of images. Thanks for the great work.

@peterfortuin
Copy link

@denisgoryaynov Is there a way to use your repository/branch into my Gatsby project with yarn instead of the standard gatsby-source-strapi package that is downloaded from npm?

@carlaiau
Copy link

carlaiau commented Apr 26, 2020 via email

@peterfortuin
Copy link

Thanks a lot @carlaiau . This would allow me to continue with my project.

Copy link
Contributor

@alexandrebodin alexandrebodin left a comment

Choose a reason for hiding this comment

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

Hi @denisgoryaynov Thank you again fro your PR,

what I noticed is that using the localeFile key we can get the Strapi image fields (alternativeText, caption...) I feel like we should even do that with single images tbh. This would make the Schema more consistent. Waht do you think ?

@denisgoryaynov
Copy link
Contributor Author

@alexandrebodin that's a good idea, I recently updated to strapi beta 20 and didn't notice that these fields were missing from single images, I'll take a look a it!

Signed-off-by: denisgoryaynov <denis.gor27@gmail.com>
@denisgoryaynov
Copy link
Contributor Author

@alexandrebodin I updated my code, it should be good now

@ordishc
Copy link

ordishc commented Aug 4, 2020

Thanks @denisgoryaynov ! Much appreciated :)

package.json Outdated
"peerDependencies": {
"gatsby-source-filesystem": "^2.x"
},
"pre-commit": [
Copy link
Contributor

Choose a reason for hiding this comment

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

you can remove the precommit it is handled by husky now

Copy link

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alexandrebodin @tonoli Sorry for the delay, I was on vacation. I pushed the updated code now

Signed-off-by: denisgoryaynov <denis.gor27@gmail.com>
@hashinteractive
Copy link

Hey guys,

Any update on this PR as we are still only able to access the thumbnail format for multiple images as a local file in Gatsby. Such as:
image

As you can see there are no local file options for multiple image formats only the thumbnail format. Also, the docs seem to be off:

{
  allStrapiArticle {
    edges {
      node {
        id
        singleImage {
         publicURL
        }
        multipleImages {
          localFile {
            publicURL
          }
        }
      }
    }
  }
}

As there is no query field localFile for multiple images.

@denisgoryaynov
Copy link
Contributor Author

@alexandrebodin @tonoli Hi, I fixed the conflict in fetch.js after the latest commit

@hashinteractive
Copy link

hashinteractive commented Sep 11, 2020

@denisgoryaynov thanks for your work! I have not tested your PR but does it address creating a local gatsby File for each of the strapi image formats (large, medium and thumbnail) or does it create a whole new field such as

node {
  multipleImages {
    localFile
  }
}

Just curious how your PR implements the fix.

@denisgoryaynov
Copy link
Contributor Author

@hashinteractive Hi, yes it does create a localFile field for every format

Schermata 2020-09-11 alle 20 59 47

What the code does is to iterate every field recursively and if it finds an image (I test for the "mime" field to detect an image as the original implementation) it generates a localFile node with publicURL and childImageSharp properties

P.S. Thank you, when doing this screenshot I found out that there was a bug when generating images for formats.
My code and the original implementation used this to generate a cache key for every image:
const mediaDataCacheKey = 'strapi-media-${item.id}'
but images inside formats do not have an id field so every key was assigned a value of: strapi-media-undefined, and showed wrong images when generating a localFile node.
Using the hash field fixes the issue:
const mediaDataCacheKey = 'strapi-media-${item.hash}'

@hashinteractive
Copy link

@denisgoryaynov thanks for your work and for the explanation. Hopefully we can get the PR merged soon 🙏

Signed-off-by: denisgoryaynov <denis.gor27@gmail.com>
@MrChrisRodriguez
Copy link

@denisgoryaynov I pulled your fork and when I build it fails because AllStrapiWebsites isn't showing up. When I revert back to origin AllStrapiWebsites appears. Have you tested your latest changes on a vanilla Gatsby-Strapi environment?

@Bubiec
Copy link

Bubiec commented Oct 19, 2020

Will this pull request fix fetching alt text from single images? I would really love to see that merged soon.

@denisgoryaynov
Copy link
Contributor Author

@MrChrisRodriguez Hi, sorry for the late response but I missed the notification. I tried by setting up a vanilla project using gatsby new frontend and npx create-strapi-app admin and as far as I can tell the build works correctly. Here are the nodes that are generated:
Schermata 2020-10-24 alle 15 58 06

And here is what I added to my package.json:
"gatsby-source-strapi": "git+https://github.com/denisgoryaynov/gatsby-source-strapi.git#build",

I forgot to update to build branch with the latest files from the master branch so that's maybe why you had issues, it should be ok now.

@Bubiec yes it does generate alternativeText for single images, you can see it from the screenshot above

@alexandrebodin alexandrebodin mentioned this pull request Nov 18, 2020
@alexandrebodin
Copy link
Contributor

Hi @denisgoryaynov. I refactored quite a lot of code and integrated changes to support what you have done.
Your code was a huge Help ! thanks a lot.

I released v1.0.0-alpha.0 with the latest changes. please try it out to validate things work for you so we can move release as latest !

@denisgoryaynov
Copy link
Contributor Author

Hi @alexandrebodin, thank you! I will try the new version as soon as I can!

@DimaMiro
Copy link

Hello 👋 How is it going with this PR? @denisgoryaynov
I do need this change in my project 🙂

@alexandrebodin
Copy link
Contributor

Hi @DimaMiro the changes of this PR were integrated in the v1 alpha release v1.0.0-alpha.0. Please try it out and give some feedback so we can move it to stable :)

@DimaMiro
Copy link

DimaMiro commented Jan 10, 2021

Hi @DimaMiro the changes of this PR were integrated in the v1 alpha release v1.0.0-alpha.0. Please try it out and give some feedback so we can move it to stable :)

Hi @alexandrebodin 👋 I've tried this change in alpha and it works correctly. Thank you 🙌

@strapi-cla
Copy link

strapi-cla commented Jan 21, 2021

CLA assistant check
All committers have signed the CLA.

@visuvelu
Copy link

Hi, how is it going with PR? @denisgoryaynov @alexandrebodin I need this change in my projects.

@alexandrebodin
Copy link
Contributor

@visuvelu if you check the comments above you will see we made the fix in the alpha version but not many people have tester it so far so I'm holding off the release. You can use it as it fixes a lot of the current issues

@Arahis
Copy link

Arahis commented Mar 26, 2021

Worked great for me. Thanks a lot!

@alexandrebodin
Copy link
Contributor

Hi, We just released the v1.0.0 fixing including this requested changes :)

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

Successfully merging this pull request may close these issues.