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.

Sign up for GitHub

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

Jump to bottom

Gutenberg: Move Related Posts to production blocks #29650

Merged
merged 1 commit into from
Jan 3, 2019
Merged

Gutenberg: Move Related Posts to production blocks #29650

merged 1 commit into from
Jan 3, 2019

Conversation

tyxla
Copy link
Member

@tyxla tyxla commented Dec 20, 2018 •
edited
Loading

DO NOT MERGE: We need to make the block available on Jetpack and WP.com first, and then get it tested:

Changes proposed in this Pull Request

  • Move Related Posts block to Production blocks.

Testing instructions

  • Follow test instructions in D19537-code

@tyxla tyxla added Jetpack [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. DO NOT MERGE [Goal] Gutenberg Working towards full integration with Gutenberg Related Posts labels Dec 20, 2018
@matticbot
Copy link
Contributor

Test live: https://calypso.live/?branch=update/gutenberg-related-posts-production

@tyxla tyxla self-assigned this Dec 20, 2018
@tyxla tyxla requested a review from ockham December 20, 2018 08:13
@tyxla tyxla added [Status] Blocked / Hold [Pri] High Address as soon as possible after BLOCKER issues labels Dec 20, 2018
@ockham
Copy link
Contributor

ockham commented Dec 20, 2018

To clarify, we also want to wait until after the CfT before merging this, correct?

@tyxla
Copy link
Member Author

tyxla commented Dec 20, 2018

If we manually update the staging blocks on .com, yes. But my plan was to do it automatically by merging this PR. It would also make testing easier, as it wouldn't require beta blocks to be enabled for folks who want to test on Jetpack sites.

@ockham
Copy link
Contributor

ockham commented Dec 20, 2018

Merging it before the CfT would essentially mean releasing the block to the public on WP.com already though, no? Which I think is the wrong order :)

Sooner or later, we should just enable beta blocks in staging on WP.com (following a suggestion by @lezama). We just might need to make sure that they don't confuse A12s, so the idea was maybe to add little 'beta' ribbons to them.

@tyxla
Copy link
Member Author

tyxla commented Dec 20, 2018

Yeah, you're right. Let's merge this when we get some testing and we know it's in a stable state.

@ockham
Copy link
Contributor

ockham commented Dec 20, 2018

You're right however that we either need to add a note to the CfT to instruct testers to apply a patch to their sandbox (ugh), or enable beta blocks on staging before posting the CfT.

@tyxla
Copy link
Member Author

tyxla commented Dec 20, 2018

enable beta blocks on staging before posting the CfT

Sounds like the best solution - we don't want to overcomplicate testing flows. Any downsides with it? Adding a "beta" ribbon can be done later.

@ockham
Copy link
Contributor

ockham commented Dec 20, 2018

I think the major downside is the potential confusion it might add for A12s, but I agree -- willing to incur that for a bit before adding whatever visual indicator later.

@tyxla tyxla force-pushed the update/gutenberg-related-posts-production branch from 9a1823d to 981dfdc Compare January 2, 2019 13:47
ockham
ockham approved these changes Jan 2, 2019
View reviewed changes
Copy link
Contributor

@ockham ockham left a comment

Choose a reason for hiding this comment

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

LGTM. I'd like to ideally wait for some feedback from our testers if some issues we weren't able to repro have been fixed by D22689-code (p1HpG7-64l-p2 #comment-29688) but let's not block it by that -- I think it's fine to aim for merging tomorrow.

@tyxla tyxla added [Status] Ready to Merge and removed DO NOT MERGE [Status] Blocked / Hold [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Jan 3, 2019
@tyxla tyxla merged commit 3e89964 into master Jan 3, 2019
@tyxla tyxla deleted the update/gutenberg-related-posts-production branch January 3, 2019 11:03
blowery added a commit that referenced this pull request Jan 3, 2019
@blowery
Merge remote-tracking branch 'origin/master' into feature/color-refre…
ebe595c 
…sh-2019

* origin/master:
  Disable select when only one domain exiists (#29873)
  Tiled galleries: Disable captions (#29776)
  Prevent scrolling up when opening a dialog (#29832)
  Gutenberg: Move Shortlinks to production (#29883)
  Gutenlypso: add convert to blocks dialog (#29790)
  Gutenberg: Move Related Posts to production blocks (#29650)
  Analytics: fix ad-tracking issue on signup for non-gdpr countries (#29741)
  Update mobile phone validation module (used for 2fa) (#29740)
  Gutenlypso: make sure titles load on second edit (#29877)
  Site Picker: Change wording of /page and /block-editor to match /post (#29859)
  Gutenberg: update copy link in page list to be editor aware
  Gutenberg: use core approach of initialEdits over overridePost
  Gutenberg: when duplicating a post, override post content
  Gutenberg: update duplicate url when Gutenlypso is enabled
  Refactor: Replace use of key-mirror with inline code (#29857)
  Fixes wrong selected domain name (#29824)
  Turn off prettier for SASS, use stylelint instead (#29697)
  Gutenberg: Add copy button to Shortlinks (#29556)
  add a section name to the body class (#29563)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ockham ockham ockham approved these changes

Assignees

@tyxla tyxla

Labels
[Goal] Gutenberg Working towards full integration with Gutenberg Jetpack [Pri] High Address as soon as possible after BLOCKER issues Related Posts
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tyxla @matticbot @ockham