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

Stories block: hide/show based on Editor capability settings being set, only after it's been loaded #2770

Merged

Conversation

mzorz
Copy link
Contributor

@mzorz mzorz commented Nov 2, 2020

We observed the settings may not be ready exactly when the native.render hooks get run but rather right after that execution cycle. Hence, we're only checking for the actual settings to be loaded by using setTimeout without delay

To test:

  • verify the tests are now passing with npm run test src/test/index.js

Note: I've considered and tried adding unit tests for the specific functionality added but, since we haven't found a way to mock select/dispatch easily yet - will get back at this at a later point.

PR submission checklist:

  • I have considered adding unit tests where possible.
  • I have considered if this change warrants user-facing release notes more info and have added them to RELEASE-NOTES.txt if necessary.

@mzorz mzorz added Testing Anything related to automated tests Story block labels Nov 2, 2020
@mzorz mzorz requested a review from etoledom November 2, 2020 21:43
@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Nov 2, 2020

Wanna run full suite of Android and iOS UI tests? Click here and 'Approve' CI job!

Copy link
Contributor

@etoledom etoledom left a comment

Choose a reason for hiding this comment

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

Working great as expected 🎉

Tested on Android, iOS and unit tests passing locally too
Great job @mzorz ! 🙏

return select( 'core/block-editor' ).getSettings( 'capabilities' )
.mediaFilesCollectionBlock;
}, [] );
// eslint-disable-next-line @wordpress/react-no-unsafe-timeout
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we could write down the reason why are we using a timeout here too for future references

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea! added here 6eb17d5

@mzorz mzorz merged commit 96a7f64 into try/jetpack-stories-block Nov 3, 2020
@mzorz mzorz deleted the try/jetpack-stories-block-fix-tests branch November 3, 2020 19:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Story block Testing Anything related to automated tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants