Skip to content

Conversation

@enejb
Copy link
Contributor

@enejb enejb commented May 20, 2021

This PR is similar to #16347

It adds the availability information that is needed for the layout grid block to be available to only .com sites for now and in the future to all sites that have the layout grid block.

Related PR:
WordPress/gutenberg#31777
Related wordpress-mobile/gutenberg-mobile#3513

To test:

CASE A:
- An business site, as well as a simple site, should see the layout grid block.

CASE B:
- An Jetpack site or a .org site should NOT see the layout grid block.

To test:

Regression Notes

N/A

PR submission checklist:

  • I have completed the Regression Notes.
  • I have considered adding unit tests for my changes.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented May 20, 2021

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented May 20, 2021

You can test the changes on this Pull Request by downloading it from AppCenter here with build number: 51010. IPA is available here. If you need access to this, you can ask a maintainer to add you.

@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented May 20, 2021

Warnings
⚠️ PR is missing at least one label.

Generated by 🚫 dangerJS

Copy link
Contributor

@chipsnyder chipsnyder left a comment

Choose a reason for hiding this comment

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

Good start here @enejb 👍 Just has a couple of minor tweaks then I can dive in and really start testing.

.mentions: FeatureFlag.gutenbergMentions.enabled && SuggestionService.shared.shouldShowSuggestions(for: post.blog),
.xposts: FeatureFlag.gutenbergXposts.enabled && SiteSuggestionService.shared.shouldShowSuggestions(for: post.blog),
.contactInfoBlock: post.blog.supports(.contactInfo) && FeatureFlag.contactInfo.enabled,
.layoutGridBlock: FeatureFlag.layoutGrid.enabled,
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this meant to also call into post.blog.supports(.layoutGrid)

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 catch this is fixed now.

@chipsnyder chipsnyder self-requested a review May 21, 2021 19:07
Copy link
Contributor

@chipsnyder chipsnyder left a comment

Choose a reason for hiding this comment

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

This doesn't build for me this time :/


- (BOOL)supportsLayoutGrid
{
if (!FeatureFlag.layoutGrid.enabled) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This PR doesn't build for me. It looks like we forgot to convert this to Objective-C. You can use the Feature class to make that change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should be fixed now.

Copy link
Contributor

@chipsnyder chipsnyder May 26, 2021

Choose a reason for hiding this comment

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

This should be fixed now.

👍 I'll do another round of testing tomorrow

@chipsnyder chipsnyder self-requested a review May 26, 2021 21:12
@enejb enejb force-pushed the add/layout-grid-block-availability branch from 4532c5c to 7098efb Compare June 7, 2021 16:55
Copy link
Contributor

@chipsnyder chipsnyder left a comment

Choose a reason for hiding this comment

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

This looks good to me now. Since we took care of the downstream issues I wonder if we even need to introduce the feature flag, or if we can just have it enabled for when the branches merge?

@mokagio
Copy link
Contributor

mokagio commented Jun 14, 2021

Just a note to say that I'm running the code freeze for 17.6 today. Before code freezing, I look at all the approved PRs and merge them.

This PR is not assigned the 17.6 milestone, though. On top of that, it has a long history and there is a suggestion for extra work / simplification from the reviewer. As such, I won't be merging it into develop before the 17.6 code freeze.

Let me know if you really need this in 17.6, so much that it can't wait two weeks, and we'll put it as a followup. 😄 👍

Thanks 🙇‍♂️

@enejb
Copy link
Contributor Author

enejb commented Jun 15, 2021

We are hoping to merge this into 17.7 now. Thanks for checking @mokagio

@enejb enejb added this to the 17.7 milestone Jun 15, 2021
@enejb enejb force-pushed the add/layout-grid-block-availability branch from 7098efb to d4cff8c Compare June 22, 2021 00:42
@enejb enejb merged commit c2246ee into develop Jun 22, 2021
@enejb enejb deleted the add/layout-grid-block-availability branch June 22, 2021 02:52
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.

4 participants