PLANET-6156: Split Spreadsheet block scripts

[lithrel]

Ref: https://jira.greenpeace.org/browse/PLANET-6156

Split Spreadsheet scripts to only be loaded when a block is present on the page.

Test

Spreadsheet block should work as expected

• After pulling this PR, rebuild JS and CSS assets
• Add a spreadsheet to a page
• Use this doc for test https://docs.google.com/spreadsheets/d/e/2PACX-1vQz5nftk5gXiehFtdxLokvInQ4au5vAMdslL--x6RMawo8Y4NgXBOlx26PzWy9heM4hytv67FkkKg4T/pubhtml
• Spreadsheet should appear on frontend and in editor as usual, with proper styling
• You can see scripts being loaded independently for Spreadsheet in you console network tab, in the editor and on the frontend

Common functions imported

For this split I checked bits webpack imported in the compiled file that are used elsewhere and that we could load globally once instead:

// only used in Spreadsheet
blocks/HighlightMatches.js
blocks/toDeclarations.js
// used elsewhere, could be exported to external dep
functions/addQueryArgs.js
functions/fetchJson.js

We could create a ticket to import all functions in our namespace in one go, p4.fetchJson(), etc. and declare it as externals for webpack ?

Generic method for following tickets

Replacing {Blockname} by PascalCase block name, and {blockname} by lowercase block name.

Styles

In assets/src/styles/blocks:

• Create styles directory
  mkdir {Blockname}
• Move existing styles
  git mv {Blockname}.scss {Blockname}/{Blockname}Style.scss
git mv {Blockname}Editor.scss {Blockname}/{Blockname}EditorStyle.scss
  If no style file exists, you don't have to create an empty one.
• Import missing styles in style files, eg:

    /* {Blockname}/{Blockname}Style.scss */
    @import "../../master-theme/assets/src/scss/base/variables";
    @import "../../master-theme/assets/src/scss/base/colors";
    @import "../../master-theme/assets/src/scss/base/mixins";

Scripts

In assets/src/blocks:

• Create script file and include render code
  touch {Blockname}/{Blockname}Script.js

import { {Blockname}Frontend } from './{Blockname}Frontend';

document.addEventListener( 'DOMContentLoaded', () => {
  const {blockname}Blocks = [...document.querySelectorAll('[data-render="planet4-blocks/{blockname}"]')];

  {blockname}Blocks.forEach(blockNode => {
    const attributes = JSON.parse( blockNode.dataset.attributes );
    wp.element.render( <{Blockname}Frontend { ...attributes.attributes } />, blockNode );
  });
});

• Remove import from assets/src/frontendIndex.js
• Rename editor script
  git mv {Blockname}/{Blockname}Editor.js {Blockname}/{Blockname}EditorScript.js
• Modify references to this file accordingly
• If necessary, edit {Blockname}/{Blockname}Block.js to export a register function instead of a class
• Modify assets/src/editorIndex.js accordingly

PHP class

• Enqueue scripts in classes/blocks/class-{blockname}.php, extract register function from constructor

	/**
	 * Constructor.
	 */
	public function __construct() {
		add_action( 'init', [ $this, 'register_{blockname}_block' ] );
	}

	/**
	 * Register block
	 */
	public function register_{blockname}_block() {
		register_block_type(
			self::get_full_block_name(),
			[...]
		);

		add_action( 'enqueue_block_editor_assets', [ self::class, 'enqueue_editor_assets' ] );
		add_action( 'wp_enqueue_scripts', [ self::class, 'enqueue_frontend_assets' ] );
	}

• If a script does not exist, explicitly declare it in the class, eg:

	/**
	 * Spreadsheet has no editor style.
	 */
	public static function enqueue_editor_style(): void {
	}

[planet-4]

Test instance is ready 🚀

🌑 oberon

⌚ 2021.05.26 11:38:27

[mleray]

Tested on local with both existing block and new block, everything seems to be working as expected 👍 However while testing I did find a few bugs/issues that are unrelated to the changes since they also happen in production:

• Trying to “unselect” a color for the block by double-clicking on it in the sidebar causes the block to crash with console error (see screenshot)
• When searching for terms in the sheet, if the string is only one character long and has no corresponding results (for example “h” in the example sheet that you provided in order to test this ticket) nothing shows instead of the expected “No data matching your search” message... This is due to this check being > 1 instead of > 0, not sure if there’s a reason for that or if it’s a mistake 🤔
• There’s also a console warning: componentWillReceiveProps has been renamed, and is not recommended for use so we should probably update that 😬

Of course these are out of the scope of this ticket, so if it's too long to solve and too many changes it's probably best to tackle them in a follow-up ticket 🙂

[lithrel]

@mleray Fixed the first 2 points, not sure the last one is fast to fix so I'll open a ticket for it.

[mleray]

Instead of adding this override here, wouldn't it be possible to check in the original function that the files exist? 🤔

[lithrel]

Yeah good point, I was thinking about the overhead of checking files on disk, but it's minimal and we serve mostly cached content, so it's easier that way. Done 👍

[mleray]

Looks good to me 👍