Gif block: refactor and add test coverage #19066

ramonjd · 2021-03-08T04:52:22Z

Changes proposed in this Pull Request:

After trying to add test coverage to the Gif block in #19019, we thought we'd preserve some sanity by refactoring the block in order to:

Make the code more readable.
Make it all more testable.
Make life more comfortable

Jetpack product discussion

p1HpG7-b3G-p2

Does this pull request change what data or activity we track or use?

Nope.

Testing instructions:

Make sure there are no visual or functional regressions in the editor or the save versions. Nothing should change in this respect.

Check that:

The search function works as expected, also try the accepted giphy URL formats, e.g., https://media.giphy.com/media/L0B6dlBlmxo6dt6YkI/giphy.gif, https://giphy.com/gifs/nfl-football-super-bowl-lv-kxrANnpcIlNiGPViUn, http://i.giphy.com/hvE0PhVAnGQAo.gif and https://giphy.com/embed/35Co9iXhs8sHgmCZFc
You can select a giphy to display from the search results' thumbnails
You can write and save a caption

Run the unit tests:

cd projects/plugins/jetpack
yarn fixtures:test extensions/blocks/gif/test

Proposed changelog entry for your changes:

Not sure if a changelog is required for a refactor and tests. No functionality/UI has changed.

GIF Block: refactored edit.js and added test coverage

matticbot · 2021-03-08T04:52:28Z

Caution: This PR has changes that must be merged to WordPress.com
Hello ramonjd! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer and confirm D58239-code works as expected before merging this PR. Once this PR is merged, please commit the changes to WP.com. Thank you!
This revision will be updated with each commit to this PR

github-actions · 2021-03-08T04:53:11Z

Thank you for your PR!

When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:

✅ Include a description of your PR changes.
✅ All commits were linted before commit.
✅ Add a "[Status]" label (In Progress, Needs Team Review, ...).
✅ Add testing instructions.
✅ Specify whether this PR includes any changes to data or privacy.

This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖

If you are an automattician, once your PR is ready for review add the "[Status] Needs Team review" label and ask someone from your team review the code.
Once you’ve done so, switch to the "[Status] Needs Review" label; someone from Jetpack Crew will then review this PR and merge it to be included in the next Jetpack release.undefined

jetpack plugin:

Next scheduled release: April 6, 2021.
Scheduled code freeze: March 29, 2021

ramonjd · 2021-03-09T20:19:20Z

projects/plugins/jetpack/extensions/blocks/gif/test/edit.js

+		expect( container.querySelector( 'figure' ) ).toBeInTheDocument();
+		expect( container.querySelector( 'figcaption' ) ).toBeInTheDocument();
+		expect( container.querySelector( '.wp-block-jetpack-gif-wrapper iframe' ) ).toBeInTheDocument();
+		expect( container.querySelectorAll( '.wp-block-jetpack-gif_thumbnail-container' ) ).toHaveLength( 2 );


Actually wondering if I should just to a toMatchSnapShot() here... 🤔

ramonjd · 2021-03-10T11:45:25Z

projects/plugins/jetpack/extensions/blocks/gif/edit.js

-			searchText
-		) }&api_key=${ encodeURIComponent( GIPHY_API_KEY ) }&limit=10`;
-	};
+	}, [ giphyData ] );


I'm aware of the missing function dependency.

Here we only care about the currency of giphyData, which changes after every API update. Otherwise it won't fire.

setSelectedGiphy is used here and when we select a new thumbnail in the results so it's going to change the function reference.

As it is, the state is only updated once and when we desire it.

We could wrap it in a useCallback, I've tried this but I think it's an optimisation we can leave for later.

Could you then add an eslint-disable and a quick explanation in the code?

Thanks for the review @jeherve

I've had a change of heart and decided to appease the linter, and have extracted the method that does reference state/props. Reference: https://reactjs.org/docs/hooks-faq.html#is-it-safe-to-omit-functions-from-the-list-of-dependencies

It might save us some bugs in the future if this block is further developed :D

jeherve · 2021-03-10T12:04:22Z

projects/plugins/jetpack/extensions/blocks/gif/edit.js

-			searchText
-		) }&api_key=${ encodeURIComponent( GIPHY_API_KEY ) }&limit=10`;
-	};
+	}, [ giphyData ] );


Could you then add an eslint-disable and a quick explanation in the code?

Importing regenerator runtime to polyfill transpiled generator functions for react hooks testing library Added custom hook to fetch api data Added tests for custom hook and updated tests for edit.js

…edGiphyAttributes is not referencing any props or state so we can use it in the useEffect hook and not reference it.

…s a little stricter than testing for the index of the domain.

jeherve

This tests well for me, nice rework! 🚢

github-actions · 2021-03-15T05:44:56Z

Great news! One last step: head over to your WordPress.com diff, D58239-code, and commit it.
Once you've done so, come back to this PR and add a comment with your changeset ID.

Thank you!

ramonjd · 2021-03-15T08:07:22Z

r222722-wpcom

matticbot added the Touches WP.com Files label Mar 8, 2021

ramonjd self-assigned this Mar 8, 2021

ramonjd added [Block] GIF [Type] Task labels Mar 8, 2021

github-actions bot added the [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ label Mar 8, 2021

github-actions bot added the [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! label Mar 8, 2021

ramonjd added the [Status] In Progress label Mar 9, 2021

ramonjd force-pushed the refactor/gif-block branch from ff5684f to cd0a1b9 Compare March 9, 2021 11:05

ramonjd added the [Status] Needs Team Review label Mar 9, 2021

ramonjd requested a review from a team March 9, 2021 11:11

ramonjd commented Mar 9, 2021

View reviewed changes

ramonjd force-pushed the refactor/gif-block branch 2 times, most recently from 1a5786b to e0326e5 Compare March 10, 2021 10:46

ramonjd commented Mar 10, 2021

View reviewed changes

jeherve requested changes Mar 10, 2021

View reviewed changes

ramonjd force-pushed the refactor/gif-block branch from 7cefa79 to 97738c9 Compare March 11, 2021 00:59

ramonjd added 8 commits March 11, 2021 12:00

Initial commit

08aea4d

Focusing forward ref input field

37d3e9e

Added tests

2a9939c

Added @testing-library/react-hooks library to unit test custom hooks

c0717d1

Importing regenerator runtime to polyfill transpiled generator functions for react hooks testing library Added custom hook to fetch api data Added tests for custom hook and updated tests for edit.js

revert css change as it would be a UI regression

50ff6af

Removing previous version (kept it for reference while developing)

1275a76

Add changelog

31a6058

Here we are extracting getSelectedGiphyAttributes to utils. getSelect…

4509013

…edGiphyAttributes is not referencing any props or state so we can use it in the useEffect hook and not reference it.

ramonjd force-pushed the refactor/gif-block branch 4 times, most recently from 7b74cdf to dcdb32d Compare March 11, 2021 02:14

Using a regex to capture known giphy paths and extract the id. This i…

575a94e

…s a little stricter than testing for the index of the domain.

ramonjd force-pushed the refactor/gif-block branch from dcdb32d to 575a94e Compare March 11, 2021 02:15

ramonjd removed the [Status] In Progress label Mar 11, 2021

ramonjd requested a review from jeherve March 11, 2021 23:53

jeherve approved these changes Mar 12, 2021

View reviewed changes

jeherve added this to the jetpack/9.6 milestone Mar 12, 2021

ramonjd merged commit 2c1efe3 into master Mar 15, 2021

ramonjd deleted the refactor/gif-block branch March 15, 2021 05:44

matticbot removed the [Status] Ready to Merge Go ahead, you can push that green button! label Mar 15, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Gif block: refactor and add test coverage #19066

Gif block: refactor and add test coverage #19066

ramonjd commented Mar 8, 2021 •

edited

Loading

matticbot commented Mar 8, 2021

github-actions bot commented Mar 8, 2021 •

edited

Loading

ramonjd Mar 9, 2021

ramonjd Mar 10, 2021

jeherve Mar 10, 2021

ramonjd Mar 11, 2021

jeherve Mar 10, 2021

jeherve left a comment

github-actions bot commented Mar 15, 2021

ramonjd commented Mar 15, 2021

Gif block: refactor and add test coverage #19066

Gif block: refactor and add test coverage #19066

Conversation

ramonjd commented Mar 8, 2021 • edited Loading

Changes proposed in this Pull Request:

Jetpack product discussion

Does this pull request change what data or activity we track or use?

Testing instructions:

Proposed changelog entry for your changes:

matticbot commented Mar 8, 2021

github-actions bot commented Mar 8, 2021 • edited Loading

ramonjd Mar 9, 2021

Choose a reason for hiding this comment

ramonjd Mar 10, 2021

Choose a reason for hiding this comment

jeherve Mar 10, 2021

Choose a reason for hiding this comment

ramonjd Mar 11, 2021

Choose a reason for hiding this comment

jeherve Mar 10, 2021

Choose a reason for hiding this comment

jeherve left a comment

Choose a reason for hiding this comment

github-actions bot commented Mar 15, 2021

ramonjd commented Mar 15, 2021

ramonjd commented Mar 8, 2021 •

edited

Loading

github-actions bot commented Mar 8, 2021 •

edited

Loading