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

Don't export the Latest Stories data in the AMP Story editor #2369

Merged
merged 3 commits into from
May 21, 2019

Conversation

kienstra
Copy link
Contributor

As @swissspidy pointed out, this data isn't needed in that case.

This uses a similar conditional to that in enqueue_block_editor_styles().

As Pascal pointed out,
this data isn't needed in that case.
@googlebot googlebot added the cla: yes Signed the Google CLA label May 21, 2019
@westonruter
Copy link
Member

Build error:

1) Test_AMP_Post_Meta_Box::test_enqueue_block_assets
Trying to get property of non-object
/tmp/wordpress/src/wp-content/plugins/amp/includes/class-amp-story-post-type.php:573
/tmp/wordpress/src/wp-content/plugins/amp/tests/test-class-amp-meta-box.php:151

kienstra added 2 commits May 21, 2019 16:00
A unit test failed,
so update it for the latest change.
Clean up the state after each test,
to ensure that there isn't a failed test.
@kienstra
Copy link
Contributor Author

Hi @westonruter, sorry about that. The build is passing now.

@westonruter
Copy link
Member

It doesn't seem this PR has any practical effect? I checked the amp-stories-redux branch and I don't see var ampLatestStoriesBlockData on the post-new.php?post_type=amp_story admin page to begin with. So is this just a matter of correctness?

@kienstra
Copy link
Contributor Author

@westonruter, good point. I also see that ampLatestStoriesBlockData isn't present in the AMP Story editor, even without this PR.

Maybe this could prevent a future regression, but otherwise it looks like this PR isn't very useful.

@swissspidy
Copy link
Collaborator

As I mentioned in #2355 (comment):

There's just no output there because amp-block-editor.js is not enqueued in that scenario. Ideally that code would live in AMP_Post_Meta_Box to prevent this confusion.

@westonruter
Copy link
Member

Ideally that code would live in AMP_Post_Meta_Box to prevent this confusion.

But I think it is preferable to keep all the AMP Stories code in this one class, as it makes it easier turn on/off.

@swissspidy
Copy link
Collaborator

Fair enough. I guess we'd need to analyze all the code at some point anyway if we want to make it easier / bulletproof to completely turn off.

@westonruter westonruter merged commit 123e01c into amp-stories-redux May 21, 2019
@westonruter westonruter deleted the update/restrict-adding-data branch May 21, 2019 22:14
@westonruter westonruter added this to the v1.2 milestone May 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Signed the Google CLA
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants