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

feat(gatsby-source-wordpress): Add keepMediaDetails #15862

Merged
merged 9 commits into from
Aug 16, 2019

Conversation

billyjacoby
Copy link
Contributor

Description

This will enable users to toggle sourceThumbnails in order to decide whether or not thumbnail info is downloaded.

I needed this in order to build an image carousel that didn't warrant the highest quality photos to speed up site load times.

Related Issues

Fixes #6509

@billyjacoby billyjacoby requested review from a team as code owners July 18, 2019 01:13
@lannonbr lannonbr changed the title Enable sourcing thumbnail information #6509 fix(gatsby-source-wordpress): Enable sourcing thumbnail information Jul 18, 2019
@lannonbr lannonbr changed the title fix(gatsby-source-wordpress): Enable sourcing thumbnail information feat(gatsby-source-wordpress): Enable sourcing thumbnail information Jul 18, 2019
@amberleyromo
Copy link
Contributor

@gatsbyjs/learning is tagged on this, but more appropriate for @gatsbyjs/core -- will leave for y'all!

Copy link
Contributor

@sidharthachatterjee sidharthachatterjee left a comment

Choose a reason for hiding this comment

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

You seem to have pushed built files in src/gatsby-node.js

Could you please push the src (without transpilation)? Also, it would be nice to add a test for this new option.

@sidharthachatterjee
Copy link
Contributor

sidharthachatterjee commented Aug 14, 2019

Closing this for now since the changes are hard to grok with transpiled code

Let's re open after pushing untranspiled code and take it from there!

@billyjacoby
Copy link
Contributor Author

I just committed the changes to my fork, sorry about that!

@billyjacoby billyjacoby reopened this Aug 15, 2019
@billyjacoby
Copy link
Contributor Author

You seem to have pushed built files in src/gatsby-node.js

Could you please push the src (without transpilation)? Also, it would be nice to add a test for this new option.

This is done!

@sidharthachatterjee
Copy link
Contributor

@billyjacoby No problem. Thanks for fixing! Could you please fix conflicts and add a test for the new option! Otherwise, looks good! 🙌🏼

@billyjacoby
Copy link
Contributor Author

@billyjacoby No problem. Thanks for fixing! Could you please fix conflicts and add a test for the new option! Otherwise, looks good! 🙌🏼

Conflicts are resolved.

As for adding tests, "it's a bit tricky to mock as it needs to access to the store/cache + would download file.." according to the existing code base. To be honest I'm not sure how I'd go about mocking all of that...

@sidharthachatterjee
Copy link
Contributor

gatsby-source-wordpress/src/normalize.js still appears to be transpiled code 😞

Could you please push the source so I can review this?

As for adding tests, "it's a bit tricky to mock as it needs to access to the store/cache + would download file.." according to the existing code base. To be honest I'm not sure how I'd go about mocking all of that...

Just took a look. Fair enough. Since this is fairly simple (only 'keeps' some keys that would otherwise be deleted) I think we can skip testing this till we figure out how to better mock the download and store etc

Copy link
Contributor

@sidharthachatterjee sidharthachatterjee left a comment

Choose a reason for hiding this comment

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

Left two comments!

packages/gatsby-source-wordpress/README.md Outdated Show resolved Hide resolved
packages/gatsby-source-wordpress/src/normalize.js Outdated Show resolved Hide resolved
@billyjacoby
Copy link
Contributor Author

gatsby-source-wordpress/src/normalize.js still appears to be transpiled code 😞

Could you please push the source so I can review this?

As for adding tests, "it's a bit tricky to mock as it needs to access to the store/cache + would download file.." according to the existing code base. To be honest I'm not sure how I'd go about mocking all of that...

Just took a look. Fair enough. Since this is fairly simple (only 'keeps' some keys that would otherwise be deleted) I think we can skip testing this till we figure out how to better mock the download and store etc

I totally just forgot to push after making the changes locally last night. Should be good to go now.

@sidharthachatterjee
Copy link
Contributor

@billyjacoby Thanks! What do you think about renaming the option?

@billyjacoby
Copy link
Contributor Author

@billyjacoby Thanks! What do you think about renaming the option?

I'm totally fine with it. That was just the most concise name I could think of, but naming is certainly not my strong suite😅 ...

@sidharthachatterjee
Copy link
Contributor

@billyjacoby Oops, I hit the wrong button hah

No worries. Naming is hard! Go ahead and make the change then and we'll get this shipped 💃

@billyjacoby
Copy link
Contributor Author

@billyjacoby Oops, I hit the wrong button hah

No worries. Naming is hard! Go ahead and make the change then and we'll get this shipped 💃

Done! I liked your name better than mine, so thanks for that! 😬

@sidharthachatterjee
Copy link
Contributor

sidharthachatterjee commented Aug 15, 2019

@billyjacoby Can you please give me push access to your fork? Just did a little clean up and tried to push

I'll add that in a follow up! Merging this in now

@sidharthachatterjee sidharthachatterjee changed the title feat(gatsby-source-wordpress): Enable sourcing thumbnail information feat(gatsby-source-wordpress): Add keepMediaDetails Aug 16, 2019
@sidharthachatterjee sidharthachatterjee merged commit 1b89f31 into gatsbyjs:master Aug 16, 2019
@@ -138,7 +142,7 @@ exports.sourceNodes = async (
touchNode,
getNode,
_auth,
reporter,
Copy link
Contributor

Choose a reason for hiding this comment

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

We still should pass reporter to normalize.downloadMediaFiles

Copy link
Contributor

Choose a reason for hiding this comment

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

Fixing that in #16670

Not published yet anyway

@sidharthachatterjee
Copy link
Contributor

Renamed this to keepMediaSizes in #16670 because I realised we were deleting sizes and not media_details

Sorry for the back and forth @billyjacoby

@sidharthachatterjee
Copy link
Contributor

Published in gatsby-source-wordpress@3.1.16

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.

[gatsby-source-wordpress] Wordpress Media Sizes
4 participants