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

Remove Stories codebase #4315

Merged
merged 26 commits into from
Mar 9, 2020
Merged

Conversation

pierlon
Copy link
Contributor

@pierlon pierlon commented Feb 19, 2020

Summary

This PR removes the Stories codebase.

Fixes #4203.

Todo

  • Remove Stories related JS tests
  • Remove Stories related PHP code
  • Remove Story templates

Checklist

  • My pull request is addressing an open issue (please create one otherwise).
  • My code is tested and passes existing tests.
  • My code follows the Engineering Guidelines (updates are often made to the guidelines, check it out periodically).

@googlebot googlebot added the cla: yes Signed the Google CLA label Feb 19, 2020
Gruntfile.js Show resolved Hide resolved
assets/src/common/helpers/index.js Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
readme.md Show resolved Hide resolved
@pierlon pierlon changed the title Enhancement/4203 stories removal Remove Stories codebase Feb 19, 2020
readme.md Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@swissspidy
Copy link
Collaborator

Video files in tests/e2e/assets can also be removed.

@pierlon
Copy link
Contributor Author

pierlon commented Feb 21, 2020

Reviews addressed in cd77370 and 97ca07a.

@pierlon
Copy link
Contributor Author

pierlon commented Feb 21, 2020

I suppose the Stories project management guidelines can also be removed?

@swissspidy
Copy link
Collaborator

@pierlon Unless you all are using those? No idea 🤷‍♂

@swissspidy
Copy link
Collaborator

Starting to look really good! No just need to address the todos.

28k deleted lines of code already, crazy!

@pierlon
Copy link
Contributor Author

pierlon commented Feb 21, 2020

@westonruter would you consider keeping the Stories deprecation admin pointer? The notice on the AMP options page will still be present.

@pierlon
Copy link
Contributor Author

pierlon commented Feb 21, 2020

Also, how much of an impact will removing the 'Experiences' option create? I know that its used in the meta[name='generator'] tag to list the enabled experiences, for example.

includes/amp-helper-functions.php Show resolved Hide resolved
'theme_support' => AMP_Theme_Support::READER_MODE_SLUG,
'supported_post_types' => [ 'post' ],
'analytics' => [],
'all_templates_supported' => true,
'supported_templates' => [ 'is_singular' ],
'enable_response_caching' => true,
'version' => AMP__VERSION,
'story_templates_version' => false,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed Story relation settings; see ceab012.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't that be a database upgrade routine instead?

This right now causes database queries on every page load.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in d67421e.

amp.php Outdated Show resolved Hide resolved
@@ -473,7 +297,7 @@ public function render_supported_templates() {
$checked = (
post_type_supports( $post_type->name, AMP_Post_Type_Support::SLUG )
||
( ! AMP_Options_Manager::is_website_experience_enabled() && in_array( $post_type->name, $supported_post_types, true ) )
Copy link
Contributor Author

Choose a reason for hiding this comment

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

With this change, the post post type will be enabled by default in the supported post types.

@pierlon
Copy link
Contributor Author

pierlon commented Feb 24, 2020

Also the welcome notice may need to be reworded; it currently states:

... this release enables rich, performant experiences for your WordPress site.

<h3><?php esc_html_e( 'From granular controls that help you create AMP content, to Core Gutenberg support, to a sanitizer that only shows visitors error-free pages, to a full error workflow for developers, this release enables rich, performant experiences for your WordPress site.', 'amp' ); ?></h3>

With the Stories experience now being removed, that technically isn't true anymore.

@pierlon pierlon marked this pull request as ready for review February 24, 2020 04:59
@@ -257,11 +257,6 @@ public function add_debug_information( $debugging_information ) {
'value' => AMP_Theme_Support::get_support_mode(),
'private' => false,
],
'amp_experiences_enabled' => [
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Site Health no longer shows the enabled experiences (since Website will be the only one).

Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch

@@ -393,8 +391,6 @@ function amp_add_generator_metadata() {
}
$content .= sprintf( '; mode=%s', $mode );

$content .= sprintf( '; experiences=%s', implode( ',', AMP_Options_Manager::get_option( 'experiences' ) ) );
Copy link
Contributor Author

Choose a reason for hiding this comment

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

experiences property is no longer shown in the meta generator tag.

@@ -379,13 +379,12 @@ function amp_init() {
add_action( 'parse_query', 'amp_correct_query_when_is_front_page' );
add_action( 'admin_bar_menu', 'amp_add_admin_bar_view_link', 100 );
add_action( 'wp_loaded', 'amp_editor_core_blocks' );
add_action( 'amp_plugin_update', 'remove_amp_story_templates' );
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Story related templates will be removed after a plugin update; see d67421e.

@@ -145,7 +118,7 @@
"lint:pkg-json": "wp-scripts lint-pkg-json --ignorePath .gitignore",
"start": "wp-scripts start",
"test": "npm-run-all --parallel test:*",
"test:e2e": "cross-env WP_BASE_URL=http://localhost:8890 node ./bin/local-env/run-e2e-tests.js --config=tests/e2e/jest.config.js",
"test:e2e": "cross-env WP_BASE_URL=http://localhost:8890 wp-scripts test-e2e --config=tests/e2e/jest.config.js",
Copy link
Contributor

Choose a reason for hiding this comment

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

Good idea to use wp-scripts

@westonruter
Copy link
Member

@pierlon Ready for merge conflicts to be resolved for merge.

@swissspidy Please approve the PR when you are satisfied.

pierlon and others added 5 commits March 7, 2020 01:36
…tories-removal

# Conflicts:
#	includes/class-amp-theme-support.php
#	package-lock.json
#	package.json
…tories-removal

# Conflicts:
#	tests/php/test-class-site-health.php
Copy link
Collaborator

@swissspidy swissspidy left a comment

Choose a reason for hiding this comment

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

Let's do it

@westonruter
Copy link
Member

westonruter commented Mar 9, 2020

@swissspidy or @pierlon would you resolve the package conflicts?

@pierlon
Copy link
Contributor Author

pierlon commented Mar 9, 2020

I can get this resolved 👷‍♂️ .

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.

Remove AMP Stories and eliminate the obsolete experiences UI
5 participants