[Gutenberg] Build a base layer for Homepage #13

miina · 2018-03-28T17:02:20Z

Fixes #12.

What does the PR contain?
Adds the following Gutenberg editor blocks:

Hero block. Consists of Header and Search with date;
Travel Angles block: this is a block purely for design (the gray background on the page). This didn't play well with being inside any other block so created it separately;
Activity List block (Browse by Activities);
Discover block (Discover heading + Display post from the blog);
Featured block: Featured destinations;
Search block: the lower search block;

Some relevant notes:

Requires Gutenberg to be installed and activated;
Created index.php file for the theme which is basically almost a copy of the HTML of the Travel static template except for the parts that are rendered by blocks;
Currently all the style is loaded as it is on the template -- in the index.php (except for a one line addition) and also the same full style is loaded to the Gutenberg editor via 'enqueue_block_editor_assets' action;
None of the blocks are currently configurable;
All the Travel-related blocks can be found by searching "Travel" in the editor's block search.

Dev-related notes

The dev-related libs are installed via package.json;
The assets/js/editor-blocks.js is generated by webpack (see webpack.config.js);
For being able to use JSX and installing everything used etc. go to the theme folder and run npm install, and then npm run dev for not having to manually rebuild the files after each JS change.
Note: that's not a fixed/required configuration and if there's a preferred way instead -- 👍

Screenshot
With the current PR adding all the blocks in the correct order the page looks like this:

Problems found

By default JSX doesn't support AMP square bracket attributes (e.g. [value]). This can be fixed by either using JS for creating the elements instead of JSX OR by defining an array of the needed attribute and then adding it to JSX. For example:

const ampValueProp = {
				'[value]': 'fields_query'
			};
<input { ...ampValueProp } />

The only property that doesn't seem to work like this is [src] -- ~~this is currently missing from the Hero block implementation~~; (Workaround added in 472ede6)

amp-mustache template's content is currently missing from the Hero block implementation -- not clear yet how exactly to implement it (quick search showed that there are custom-created solutions out there); (Fixed within 97bbaf2)
At this moment the Gutenberg editor preview design is a little bit "off", especially the Featured Destinations area;

TODO / Next steps

Use only needed style for Gutenberg instead of everything;
Make editor preview look more similar to the actual page;
Hero block issues: ~~amp-mustache implementation~~ (Fixed within 97bbaf2) + Figure out how to fix the [src] not being rendered (Workaround added in 472ede6);
Make all the blocks configurable (e.g. for starters - just the texts maybe)?
If there's another preferred configuration instead of the one used in the PR, change that;

…er, featured, and search block.

miina · 2018-03-28T18:57:59Z

There were a few issues that came out while adding the base layout for the homepage, the suggested issues to continue with are:

Fixing the Gutenberg editor preview style (make it match better the frontend + use only necessary style, not everything);
Hero block: ~~figure out the implementation of amp-mustache~~ (Fixed within 97bbaf2), and also how to fix the [src] not being rendered (Workaround added in 472ede6);
Make the main blocks configurable (Hero, Activity List, Discover, Featured, Search);

Thinking that perhaps it would make sense to try figuring out the implementation of ~~amp-mustache and~~ [src] first, maybe the editor-side issues are less important (?).Thoughts?

@ThierryA Would be great to hear your opinion.

Edit: Added a workaround for the [src] issue in 472ede6.

cc @DavidCramer @postphotos

miina · 2018-03-29T12:28:29Z

blocks/hero/index.js

+			// @todo Confirm possible solution for replacing [src] -- add data-ampsrc instead and replace it with filters when displaying the post.
+			const ampSrcProp = {
+				'data-ampsrc': "'/api/places?types=(regions)&amp;types=(cities)&amp;input=' + fields_query_live"
+			};


React is allowing the rest of the amp attributes to be saved ([value] etc.) but [src] not. Added a workaround which uses data-ampsrc instead and then this will later get replaced with the_content filter when rendering the post. Does this seem reasonable?

This is essentially what we're having to do in PHP as well to support these attributes. Before sending into DOMDocument to parse, we replace all such attributes with data- attributes and then after parsing is done we convert them back to the original bracketed-notation. See: https://github.com/Automattic/amp-wp/blob/2bcfc19c2108b9b7bdd5c2eab1532e90a2bb0882/includes/utils/class-amp-dom-utils.php#L182-L272

I think what you have is more or less what we have to live with until AMP adds support for an XHTML syntax: ampproject/amphtml#11115

miina · 2018-03-29T13:23:14Z

The Gutenberg base layout PR should be ready for initial review.

Perhaps the next Gutenberg-related issues could be:

Improving blocks by making them configurable / functional (this could be an issue per block);
Improving Gutenberg editor style (removing unnecessary style and match it with the front-end as much as possible);

@ThierryA Any thoughts on priorities for continuing with Travel Gutenberg blocks? Would it make sense to already start working on making the blocks configurable / dynamic before this PR gets reviewed? Or perhaps it would make sense to improve the editor style within this PR already. Thoughts?

miina · 2018-03-30T06:40:38Z

Missing functionality

@postphotos Just in case it could be helpful for creating block-based tickets, writing out the missing functionality per block that would be required for the blocks be functional / dynamic;

Hero block (Work In Progress within Hero block for Homepage #16 ):

The texts should be editable (currently has static texts);
The API URl for pulling prediction results should be correct (currently pulling from static sample file);
Searching should direct to search results page (with relevant results);
(Also, Fix the editor side design;)

Discover block (Fixed within https://github.com/xwp/travel/tree/feature/discover_block):

The texts should be editable (currently has static texts);
Should pull the latests post dynamically;

Popular block:

The heading should be editable;
Should pull the 3 top results dynamically;

Featured destinations:

The heading should be editable;
Should display the content (CPT categories) dynamically;
(Also, fix the editor side design);

Activity List (Browse by activity):

The heading should be editable;
The links should direct to search results page (with correct results);

Search

The texts should be editable;
Should direct to search results page;
(Also, fix the editor side design);

Note that not all the related functionality can be directly added to the blocks (e.g. the search results page itself).

ThierryA

Thanks @miina, great work here.

Would it make sense to already start working on making the blocks configurable / dynamic before this PR gets reviewed

Yes all blocks should be configurable and dynamic before reviewing the PR.

I also merged the latest from develop which includes Travis integration with WPCS and ESLint sniffers set. There are a few issues that Travis picks up in the PR which must be addressed. I would suggest to also install the pre-commit hook locally, here are the steps to follow:

git pull to get the last commit
run npm install again to install ESLint and the Dev Lib
run composer install to install standardized packages (best to mimic Travis env)
install pre-commit hook ./node_modules/wp-dev-lib/install-pre-commit-hook.sh

Happy to answer any question you may have indeed.
Thanks,

ThierryA · 2018-03-30T08:50:11Z

.gitignore

@@ -0,0 +1,3 @@
+/node_modules
+/assets


I think we should checkout the compiled file as the blocks should be available without having to run npm commands. I added it in my last commit when merging develop, however I did not commit the assets yet which will be added next time you push a commit once you have pull the latest.

Added the assets to version control within f7ada7e.

miina · 2018-03-30T11:09:38Z

Thank you for reviewing the PR, @ThierryA.

The assets are now under version control as well and fixed some CS issues. Currently just added ignore to the index.php file since this is kind of a copy of the non-blocks related HTML and includes some CS issues. At this moment Travis CI is still failing but looks like it's due to PHPUnit tests configuration:
Cannot open file "/tmp/wordpress/src/wp-content/themes/travel/tests/bootstrap.php".

On making the blocks configurable / dynamic -- perhaps this can be done block-by-block in separate issues; so far there is one block-specific github issue created (#14), started working on it separately on this branch.

ThierryA · 2018-03-30T13:38:36Z

.gitignore

@@ -1,10 +1,5 @@
-<<<<<<< HEAD


Thanks for fixing this conflict @miina.

ThierryA · 2018-03-30T13:55:25Z

At this moment Travis CI is still failing but looks like it's due to PHPUnit tests configuration:
Cannot open file "/tmp/wordpress/src/wp-content/themes/travel/tests/bootstrap.php".

Thanks for picking that up @miina, this is fixed in my last commit (merged develop)

miina · 2018-04-03T16:21:12Z

@westonruter If you happen to have time would appreciate your feedback on the PR, to see if there seems to be anything to change. This is basically the base for Gutenberg blocks that creates static blocks, the dynamic/customizable block-related PR-s are/will be based on this PR.

…mplification

westonruter · 2018-04-05T01:11:44Z

The build failure for d46ede6 is good because it shows that ESLint is successfully running. Apparently dev-lib isn't yet accounting for config files named .eslintrc.js so it has to be named .eslintrc for now.

… all the blocks.

miina · 2018-04-05T08:31:39Z

blocks/amp-transformer.js

+		if ( -1 === propsToRemove.indexOf( prop ) ) {
+
+			// Eslint is giving "There should be no spaces inside this paren." error for the line.
+			if ( propsMapping[ prop ] ) { // eslint-disable-line


For some reason eslint is throwing There should be no spaces inside this paren. (space-in-parens) error for this line. Without the spaces it's throwing spaces missing error. Not sure what I'm missing here, any thoughts?

I've noticed that too. Seems like a bug in eslint.

…ml tags for theme-related tags.

miina · 2018-04-05T14:28:05Z

@westonruter:
Based on Thierry's comment moved the blocks-related logic to a separate file from functions.php.

Saw your comment on the allowed HTML tags:

// @todo WARNING: If the user does not have unfiltered_html, then the amp-img elements will be removed from the saved post_content! Should wp_kses_allowed_html be filtered to allow all AMP elements and attributes that will be used in blocks here?

Added a filter to allow amp-img and amp-list, these are the ones currently used within the theme, perhaps there is no need to add other tags unless required? Or is the theme intended to allow adding any other common amp-tags to the content? Thoughts?

westonruter · 2018-04-05T16:21:28Z

includes/class-amp-travel-blocks.php

+	public function filter_wp_kses_allowed_html( $allowed_tags, $context ) {
+		if ( 'post' === $context ) {
+			$amp_tags = array(
+				'amp-img'  => array(


This should probably be merging with $allowed_tags['img'] for good measure.

westonruter · 2018-04-05T16:21:42Z

Added a filter to allow amp-img and amp-list, these are the ones currently used within the theme, perhaps there is no need to add other tags unless required? Or is the theme intended to allow adding any other common amp-tags to the content? Thoughts?

Good. I think that's all that is needed for now. We'll need to probably handle this in the AMP plugin generally. For example, when creating AMP-specific blocks in the plugin we'll also need to make sure that all AMP elements and attributes get whitelisted in Kses, similar to how you've now done here.

kienstra

Looks Good, Comments About File Structure

Hi @miina,
This PR looks good, and thanks for putting the blocks logic in includes/class-amp-travel-blocks.php.

Could you please see the comments below? These are mainly to pave the way for more classes in includes/.

kienstra · 2018-04-10T18:05:45Z

includes/class-amp-travel-blocks.php

+	 * AMP_Travel_Blocks constructor.
+	 */
+	public function __construct() {
+		if ( function_exists( 'gutenberg_init' ) ) {


@miina, could you please create an init() method, and move the add_action() and add_filter() calls inside it?

Thanks, the change looks good.

kienstra · 2018-04-10T18:07:44Z

functions.php

+require_once get_template_directory() . '/includes/class-amp-travel-blocks.php';
+
+
+if ( ! function_exists( 'travel_setup' ) ) :


@miina,
Thanks for putting the blocks logic in a separate class.

Could you create another file, includes/functions.php, which has a function like:

function amp_travel_theme() { return AMP_Travel_Theme::get_instance(); }

Then, the file that this comment is attached to could simply call amp_travel_theme()->init() after requiring the files above. (This borrows heavily from @ThierryA's work on another project).

Then, the current logic inside if ( ! function_exists( 'travel_setup' ) ) : could move into a new AMP_Travel_Theme class: includes/class-amp-travel-theme.php. That logic could be inside a method that AMP_Travel_Theme::init() calls.

It'd be great if you could also create a method AMP_Travel_Theme::instantiate_classes(), and move the line new AMP_Travel_Blocks(); into it.

@kienstra Thanks for the comments, the file structure issues should be addressed now, let me know if there's anything else that might need changing.

Thanks a lot for restructuring the files, @miina. The structure looks good. This will pave the way for more theme classes.

kienstra

Approved

Hi @miina,
Thanks a lot for your Gutenberg expertise here, and for your great work.

ThierryA · 2018-04-11T16:29:22Z

Great work here!

miina added 10 commits March 27, 2018 11:51

Add base for Gutenberg blocks + blocks base for activity-list, discov…

7cfcb28

…er, featured, and search block.

Empty index.

5876ef3

Remove featured block.

2c6744d

Add index.php file placeholder. Add copied style from Travel theme.

c2cf477

Add featured block.

4d74f0d

Add Popular block.

f963aac

Add Hero block.

bd1ab4a

Add Travel Angle block for design.

5e8e3aa

Improve block's name.

59f25d5

Fix some CS issues.

4480d69

miina added 3 commits March 29, 2018 11:59

Add Mustache template to Hero block.

97bbaf2

Add demo.json file for amp-list. Fix some editor validation issues.

0eac19d

Add a workaround for React not supporting [src].

472ede6

miina commented Mar 29, 2018

View reviewed changes

Merge branch 'develop' into feature/12-gutenberg_blocks_base

d301cc0

ThierryA suggested changes Mar 30, 2018

View reviewed changes

Fix some CS issues.

f7ada7e

Fix gitignore.

5f74431

miina added a commit that referenced this pull request Mar 30, 2018

Merge from #13

32bf5ed

miina mentioned this pull request Mar 30, 2018

Hero block for Homepage #16

Merged

7 tasks

ThierryA reviewed Mar 30, 2018

View reviewed changes

.gitignore Outdated

@@ -1,10 +1,5 @@

<<<<<<< HEAD

Copy link

Contributor

ThierryA Mar 30, 2018

Choose a reason for hiding this comment

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

Thanks for fixing this conflict @miina.

Merge branch 'develop' into feature/12-gutenberg_blocks_base

876899b

This was referenced Apr 3, 2018

[Gutenberg] Search block for Homepage #18

Merged

[Gutenberg] Discover block for Homepage #19

Merged

Eliminate duplication of static markup for editor by doing basic de-a…

2feb47a

…mplification

westonruter force-pushed the feature/12-gutenberg_blocks_base branch from d5ef7b3 to 2feb47a Compare April 5, 2018 00:57

Fix dev-lib being able to run eslint; fix some eslint issues

d46ede6

westonruter force-pushed the feature/12-gutenberg_blocks_base branch from 7a14eec to d46ede6 Compare April 5, 2018 01:04

miina added 2 commits April 5, 2018 09:57

Fix eslint issues.

b18c6c4

Fix AMP validation issues. Reuse the same method for edit-save within…

ca8c0ce

… all the blocks.

miina commented Apr 5, 2018

View reviewed changes

miina added 2 commits April 5, 2018 17:20

Move blocks-related code away from functions.php file. Add allowed ht…

946e1f5

…ml tags for theme-related tags.

Remove comment. Fix phpcs.

2edb79a

westonruter reviewed Apr 5, 2018

View reviewed changes

miina added 2 commits April 6, 2018 14:40

Merge amp-img allowed atts with img.

81529db

Restructure.

ff30b34

kienstra suggested changes Apr 10, 2018

View reviewed changes

kienstra assigned miina Apr 10, 2018

miina added 2 commits April 11, 2018 10:34

Restructure theme code.

f9d9361

Add init method to blocks class.

7f23e78

miina assigned kienstra and unassigned kienstra Apr 11, 2018

miina mentioned this pull request Apr 11, 2018

Add Activity and Location taxonomies. #31

Merged

kienstra approved these changes Apr 11, 2018

View reviewed changes

ThierryA approved these changes Apr 11, 2018

View reviewed changes

kienstra merged commit 4d78f77 into develop Apr 11, 2018

kienstra deleted the feature/12-gutenberg_blocks_base branch April 11, 2018 16:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Gutenberg] Build a base layer for Homepage #13

[Gutenberg] Build a base layer for Homepage #13

miina commented Mar 28, 2018 •

edited

Loading

miina commented Mar 28, 2018 •

edited

Loading

miina Mar 29, 2018

westonruter Apr 4, 2018

miina commented Mar 29, 2018 •

edited

Loading

miina commented Mar 30, 2018 •

edited

Loading

ThierryA left a comment •

edited

Loading

ThierryA Mar 30, 2018

miina Mar 30, 2018

miina commented Mar 30, 2018

ThierryA Mar 30, 2018

ThierryA commented Mar 30, 2018

miina commented Apr 3, 2018

westonruter commented Apr 5, 2018

miina Apr 5, 2018

westonruter Apr 5, 2018

miina commented Apr 5, 2018 •

edited

Loading

westonruter Apr 5, 2018

westonruter commented Apr 5, 2018

kienstra left a comment

kienstra Apr 10, 2018

kienstra Apr 11, 2018

kienstra Apr 10, 2018 •

edited

Loading

miina Apr 11, 2018

kienstra Apr 11, 2018 •

edited

Loading

kienstra left a comment

ThierryA commented Apr 11, 2018

		require_once get_template_directory() . '/includes/class-amp-travel-blocks.php';


		if ( ! function_exists( 'travel_setup' ) ) :

		@@ -0,0 +1,3 @@
		/node_modules
		/assets

[Gutenberg] Build a base layer for Homepage #13

[Gutenberg] Build a base layer for Homepage #13

Conversation

miina commented Mar 28, 2018 • edited Loading

miina commented Mar 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miina commented Mar 29, 2018 • edited Loading

miina commented Mar 30, 2018 • edited Loading

ThierryA left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miina commented Mar 30, 2018

Choose a reason for hiding this comment

ThierryA commented Mar 30, 2018

miina commented Apr 3, 2018

westonruter commented Apr 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miina commented Apr 5, 2018 • edited Loading

Choose a reason for hiding this comment

westonruter commented Apr 5, 2018

kienstra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kienstra Apr 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kienstra Apr 11, 2018 • edited Loading

Choose a reason for hiding this comment

kienstra left a comment

Choose a reason for hiding this comment

ThierryA commented Apr 11, 2018

miina commented Mar 28, 2018 •

edited

Loading

miina commented Mar 28, 2018 •

edited

Loading

miina commented Mar 29, 2018 •

edited

Loading

miina commented Mar 30, 2018 •

edited

Loading

ThierryA left a comment •

edited

Loading

miina commented Apr 5, 2018 •

edited

Loading

kienstra Apr 10, 2018 •

edited

Loading

kienstra Apr 11, 2018 •

edited

Loading