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

Featured Image: Refactor Featured Image to use withApiData HoC #2527

Merged
merged 4 commits into from
Aug 25, 2017

Conversation

youknowriad
Copy link
Contributor

In this PR, I'm trying to use withAPIData in the featured image component and this is a huge improvement over repeating the same lifecycle calls over and over again :)

I've noticed a bug and I suspect that the HoC doesn't trigger the requests properly when the URL changes. I may be wrong though. (Try changing the featured image, the new one is not fetched) Any idea @aduth

@youknowriad youknowriad added the General Interface Parts of the UI which don't fall neatly under other labels. label Aug 24, 2017
@youknowriad youknowriad self-assigned this Aug 24, 2017
@youknowriad youknowriad requested a review from aduth August 24, 2017 14:10
@codecov
Copy link

codecov bot commented Aug 24, 2017

Codecov Report

Merging #2527 into master will increase coverage by 1.54%.
The diff coverage is 10%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2527      +/-   ##
==========================================
+ Coverage   28.49%   30.04%   +1.54%     
==========================================
  Files         169      174       +5     
  Lines        5148     5505     +357     
  Branches      859      981     +122     
==========================================
+ Hits         1467     1654     +187     
- Misses       3119     3234     +115     
- Partials      562      617      +55
Impacted Files Coverage Δ
editor/sidebar/featured-image/index.js 0% <0%> (ø) ⬆️
components/higher-order/with-api-data/index.js 82.66% <100%> (ø) ⬆️
blocks/library/audio/index.js 19.04% <0%> (-5.96%) ⬇️
blocks/library/html/index.js 19.04% <0%> (-4.03%) ⬇️
blocks/api/paste/utils.js 98.11% <0%> (-1.89%) ⬇️
blocks/library/cover-image/index.js 28.57% <0%> (-1.87%) ⬇️
blocks/editable/index.js 9.57% <0%> (-1.72%) ⬇️
blocks/api/paste/is-inline-content.js 100% <0%> (ø) ⬆️
blocks/api/paste/strip-attributes.js 100% <0%> (ø) ⬆️
... and 17 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2c6953e...34b32c2. Read the comment docs.

@youknowriad
Copy link
Contributor Author

@aduth The refetch issue should be fixed with 62fc297 but I'd appreciate a validation here. withAPIData is the easiest code to follow :)

@aduth
Copy link
Member

aduth commented Aug 24, 2017

The withAPIData changes make sense to me. I've added a test case in my local environment but am running into issues where the Jest tests run forever. I'm still investigating...

@aduth
Copy link
Member

aduth commented Aug 25, 2017

I discovered the issue to be an infinite loop in my rememo library 😬 Published the fix in a patch release, so should be reflected in Gutenberg (will updated #2512 accordingly).

onSelect={ onUpdateImage }
type="image"
>
{ media && !! media.data &&
Copy link
Member

Choose a reason for hiding this comment

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

media should always be an object assigned by withAPIData, so shouldn't need to test truthiness (except for testing convenience).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, but sometimes I don't assign the media prop at all in the HoC. (When there's no image set)

Copy link
Member

Choose a reason for hiding this comment

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

True. In this particular case since we're already in the condition of !! featuredImageId that would never apply, but probably good to be safe.

@@ -135,4 +79,19 @@ export default connect(
},
};
}
);

const fetchAPIData = withAPIData( ( { featuredImageId } ) => {
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to use the naming convention discussed at #2525 (comment) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I was waiting for your changes there :). Updating

@youknowriad youknowriad merged commit 6a9db6c into master Aug 25, 2017
@youknowriad youknowriad deleted the update/refactor-feature-image-with-api-data branch August 25, 2017 16:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
General Interface Parts of the UI which don't fall neatly under other labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants