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

Jetpack Tiled Gallery Block : save functionality submodule hash update #4143

Merged
merged 8 commits into from
Nov 2, 2021

Conversation

illusaen
Copy link
Contributor

@illusaen illusaen commented Oct 21, 2021

Updating submodule hash for Jetpack Tiled Gallery block's save functionality.
See details in: #21481

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.

@illusaen illusaen requested a review from guarani October 21, 2021 01:47
@illusaen illusaen changed the base branch from develop to add/tiled-gallery-block October 21, 2021 01:47
Copy link
Contributor

@guarani guarani left a comment

Choose a reason for hiding this comment

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

I see some failing tests – it looks like our integration tests for the Tiled Gallery caught an issue (or maybe they need to be updated).

@illusaen
Copy link
Contributor Author

@guarani Leaving a note here as a reminder to myself (and everyone):

Issue with this right now is that the unit test is failing with a max recursion hit error; somewhere the state is being set over and over (and thus calling render over and over) when run from the unit test but not when run manually.

As per our discussion on Slack, do we want to merge this to our feature branch? We can open a new PR for the unit test bugfix.

@guarani
Copy link
Contributor

guarani commented Oct 26, 2021

👋 @illusaen Thanks for sharing the info here about the failing test.

As per our discussion on Slack, do we want to merge this to our feature branch? We can open a new PR for the unit test bugfix.

If we merge as-is, would this failing test cause future PRs targeting the feature branch to fail? If so, would it make sense to disable the test in this PR and then merge it? We could create a new issue to fix the bug (and re-enable the test).

@illusaen
Copy link
Contributor Author

If we merge as-is, would this failing test cause future PRs targeting the feature branch to fail? If so, would it make sense to disable the test in this PR and then merge it? We could create a new issue to fix the bug (and re-enable the test).

@guarani Yes that sounds good. Did some elimination and it's definitely got something to do with the columns attribute; I think the useEffect calls are too spread out and need to be consolidated together so we can really encapsulate what we want.

For example:

  • On inserting a new tiled gallery block, our columns needs to be set to the default, i.e. 2.
  • On loading a serialized block, our columns needs to be set to:
    • if there are no images in serialized block, set to default
    • if there are images, set to Math.max(images.length, default)
  • When adding a photo to the tiled gallery block, we need to update the max columns that are allowed (and current # of columns to be under that max if necessary).
  • When removing a photo from the tiled gallery block, we likewise need to update the max columns that are allowed (and current # of columns to be under that max if necessary).

@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Oct 27, 2021

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

@illusaen
Copy link
Contributor Author

Skipping unit test.

@guarani
Copy link
Contributor

guarani commented Oct 28, 2021

Thanks for sharing those insights, @illusaen!

@guarani
Copy link
Contributor

guarani commented Nov 2, 2021

I've just updated our feature branches with the latest from Gutenberg trunk and will update this branch in the hopes that doing so will fix the build failure.

@guarani guarani merged commit 123fda4 into add/tiled-gallery-block Nov 2, 2021
@guarani guarani deleted the tiled-gallery-save-workaround branch November 2, 2021 13:57
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.

2 participants