Skip to content
This repository was archived by the owner on Feb 4, 2025. It is now read-only.

Adds supported blocks parameter in FETCH_BLOCK_LAYOUTS action#1703

Merged
antonis merged 2 commits intodevelopfrom
issue/mlp_supportedblocks
Oct 13, 2020
Merged

Adds supported blocks parameter in FETCH_BLOCK_LAYOUTS action#1703
antonis merged 2 commits intodevelopfrom
issue/mlp_supportedblocks

Conversation

@antonis
Copy link
Copy Markdown

@antonis antonis commented Sep 25, 2020

Fixes wordpress-mobile/gutenberg-mobile#2444

Depends on: #1702

Description
This PR fetches adds supported blocks parameter in fetch block layouts call both for WPCom and self hosted site

To test
This can be tested with wordpress-mobile/WordPress-Android#13018
or by running the testFetchBlockLayouts() tests.

Copy link
Copy Markdown
Contributor

@malinajirka malinajirka left a comment

Choose a reason for hiding this comment

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

LGTM, thanks! I've left one minor comment.

BuildConfig.TEST_WPORG_URL_SH_SIMPLE_ENDPOINT);

SiteModel firstSite = mSiteStore.getSites().get(0);
List<String> supportedBlocks =
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Will we need to update this list whenever we add a support for a new block?

We might want to consider passing just three basic blocks. We are testing that the API is reachable and the payload is formatted as expected. Not that the result for all these blocks is what we expect it to be, right? Wdyt?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Hello @malinajirka 👋
I agree that this test might be fragile. It will break if we remove all the layouts that contain only a subset of the supportedBlocks.
To my understanding adding new blocks will not break the test unless we include those in layouts and the above condition is not longer true.
I'm not sure that there is a layout that includes just three blocks.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I see. Makes sense, let's keep it as is and modify it only if we find the need. Thanks!

Base automatically changed from issue/selfhosted_block_layouts to develop October 12, 2020 15:07
@antonis antonis merged commit b4be2fe into develop Oct 13, 2020
@antonis antonis deleted the issue/mlp_supportedblocks branch October 13, 2020 05:46
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants