[Gutenberg] Add "Featured Destinations" homepage block #34

miina · 2018-04-06T13:24:07Z

~~Based on #12 and #31.~~

Aims to do the following:

Pull data dynamically to editor.
Sorts and renders the terms according to the uploaded images.
Register and add relevant meta to location term.

miina · 2018-04-06T13:30:06Z

@DavidCramer Started adding the featured block within this commit: 9f745ae
Basically just registered the block and meta for amp_travel_featured and enabled fetching the terms via API by the value of the meta. Also added the logic for when there are not enough featured locations found.

What's missing:

Rendering for when there are enough featured locations found (Completely missing for frontend in PHP and partly missing in JS for editor);
Adding image-related metafield(s) for the user to upload the image;
Location term field to mark it being Featured;
Sorting the images for putting together the grid.

…o feature/25-featured_block

kienstra

Nice Work
Some Points Below

Hi @miina,
Good work here. Especially in setting up the grid:

kienstra · 2018-04-13T19:22:46Z

includes/class-amp-travel-taxonomies.php

+		<div class='form-field form-required location-cover-image-wrap'>
+			<label for='location-cover-image'><?php esc_html_e( 'Activity Cover Image' ); ?></label>
+			<input type='hidden' id='location-cover-image-value' class='small-text' name='location-cover-image-value' value='' />
+			<input type='button' id='location-cover-image' class='button location-cover-image-upload-button' value='<?php esc_html_e( 'Upload' ); ?>' />


Could the "Upload" text for the button instead be something like "Select"? It's also possible to select an image from the media library.

Thanks, the new "Select" text looks good.

kienstra · 2018-04-13T19:27:00Z

assets/js/term-controls.js

+
+		} );
+
+		component.removeButton.on( 'click', function( event ) {


If you have time, could you display this component.removeButton only if there's no selected image? But I'd call this a "nice to have."

It could be confusing to have a "Remove" button if there's no image to remove.

Thanks for making this change, @miina. This works well.

kienstra · 2018-04-13T19:41:29Z

assets/js/term-controls.js

+	/**
+	 * Init.
+	 */
+	component.init = function init() {


It looks like the selected image is persisting even after saving a new "Location"

Go to /wp-admin/edit-tags.php?taxonomy=location

Fill in all of the fields, and select an image

Click "Add New Location"

Expected: the selected image doesn't display anymore, as the rest of the fields are empty

Actual: the selected image still displays:

But the workflow looks fine when editing a location:

Thanks for fixing this.

kienstra · 2018-04-13T19:54:13Z

blocks/featured/index.js

+				featuredLocations: '/wp/v2/locations?per_page=6&meta_value=1&meta_key=amp_travel_featured'
+			};
+		} )( ( { featuredLocations } ) => { // eslint-disable-line
+			const hasLocations = Array.isArray( featuredLocations.data ) && 6 === featuredLocations.data.length;


What do you think about making this

featuredLocations.data.length >= 6

instead of:

6 === featuredLocations.data.length

It might be easy for someone to change AMP_Travel_Blocks::$featured_locations_count, without noticing that this line requires it to be exactly 6.

kienstra · 2018-04-13T19:57:00Z

includes/class-amp-travel-taxonomies.php

+	 */
+	public function add_location_meta_fields() {
+		?>
+		<div class='form-field form-required location-cover-image-wrap'>


Could this method please use double quotes for the attributes, like:

<div class="form-field form-required location-cover-image-wrap">

And likewise for edit_location_meta_fields()

kienstra · 2018-04-13T23:19:57Z

includes/class-amp-travel-taxonomies.php

+			<td>
+				<?php wp_nonce_field( basename( __FILE__ ), 'travel_location_meta_nonce' ); ?>
+
+				<input type='hidden' id='location-cover-image-value' class='small-text' name='location-cover-image-value' value='<?php esc_attr( $cover_img_id ); ?>' />


<?php esc_attr( $cover_img_id ); ?>

This could probably use an echo at the beginning:

<?php echo esc_attr( $cover_img_id ); ?>

kienstra · 2018-04-13T23:33:49Z

includes/class-amp-travel-blocks.php

+			$location_img_srcset = wp_get_attachment_image_srcset( $location_img_id, 'full' );
+
+			$output .= '<a href="' . esc_url( get_term_link( $location['term_id'] ) ) . '" class="travel-featured-tile flex flex-auto relative travel-featured-color-' .
+							esc_html( $location_params[ $i ]['color'] ) . '">


esc_html( $location_params[ $i ]['color']

Could this use esc_attr() instead, as this is for an attribute (class)

kienstra · 2018-04-13T23:35:20Z

includes/class-amp-travel-blocks.php

+							esc_html( $location_params[ $i ]['color'] ) . '">
+							<amp-img class="travel-object-cover flex-auto" layout="responsive" width="' .
+							esc_html( $location_params[ $i ]['width'] ) . '" height="' .
+							esc_html( $location_params[ $i ]['height'] ) . '" srcset="' . esc_html( $location_img_srcset ) . '" src="' . esc_url( $location_img_src[0] ) . '""></amp-img>


Could this line and the line above use esc_attr() for all of the attributes? But maybe there was an issue in using esc_attr() with the srcset.

kienstra · 2018-04-13T23:44:20Z

blocks/featured/index.js

+								icon="admin-post"
+								label={ __( 'Locations' ) }
+					>
+						{ __( 'Not enough featured locations found, add at least six to use the block.' ) }


Would it be easier to use if the message mentioned "Location" terms, maybe like:

Not enough featured locations found. Please add at least six "Locations" terms, select an image, and check "Featured destination."

kienstra · 2018-04-13T23:50:32Z

includes/class-amp-travel-blocks.php

+	 *
+	 * @var int
+	 */
+	public static $featured_locations_count = 6;


This class is instantiated, so would there be a need to make this static? If not, could you please remove static, and change this to something like:

const FEATURED_LOCATIONS_COUNT = 6;

Then, references to it could change from self::$featured_locations_count to self::FEATURED_LOCATIONS_COUNT

This is outside the diff of this PR, but could you please do the same thing to $popular_posts_count below?

kienstra · 2018-04-14T00:42:38Z

Editor Styling

This is probably outside the scope of this PR. But we might revisit the styling of this block in the editor. The images appear much taller than on the front-end.

miina · 2018-04-16T12:59:06Z

@kienstra Thanks for all the found issues! These should be addressed now, also improved the Gutenberg styling a little bit.

kienstra · 2018-04-16T21:02:30Z

Approved, Other Than Merge Conflicts

Hi @miina,
Thanks for all of your work here. And thanks improving the styling of this "Featured Destinations" block in the editor.

Could you please resolve these merge conflicts? I caused them by merging in #16.

I tried to resolve them on my own, but it looks like editor-blocks.js will need to be rebuilt. And this command didn't seem to rebuild it properly:

npm run build

I probably missed something 😄

Thanks!

miina · 2018-04-17T06:28:26Z

@kienstra, Merged develop & resolved conflicts, sorry for not noticing the conflicts before.

Before, there was an error in Gutenberg, as renderStaticFeaturedBlock() wasn't defined. This PR removed that function. So change save() to return null, as this looks to be a dynamic block. AMP_Travel_Blocks::render_block_travel_featured() renders the output on the front-end. Also, compile js/editor-blocks.js, using: npm run build.

kienstra · 2018-04-17T21:05:00Z

Merging, Request For Sanity Check

Hi @miina,
Thanks, this block looks great. No need to apologize for not noticing the merge conflicts. I introduced them after your work day.

Could you please sanity-check this commit? I changed the save() function to return null. And I recompiled editor-blocks.js with npm run build.

miina · 2018-04-18T08:25:43Z

Hey @kienstra, the commit you did looks good, thanks for that!

Register featured block. Pull data dynamically. Merge #31

9f745ae

miina and others added 5 commits April 6, 2018 16:37

Fix API request URL.

29b6ca5

Fix typo

51845c0

Add img meta to locations. Sort REST response for the editor.

1085dc7

Add JS file for term controls.

ec20263

Merge branch 'feature/25-featured_block' of github.com:xwp/travel int…

d12ad1b

…o feature/25-featured_block

miina mentioned this pull request Apr 12, 2018

[Gutenberg] Build a "Featured Destinations" block for Homepage #25

Open

miina added 5 commits April 12, 2018 18:14

Merge develop & resolve conflicts.

bafa358

Add featured destination meta.

d00e00b

Merge develop & fix conflicts.

b4fc6fc

Add front-end PHP render for featured block.

72ad5a4

Fix editor-side rendering.

c984df8

miina changed the title ~~[WIP] [Gutenberg] Add "Featured Destinations" homepage block~~ [Gutenberg] Add "Featured Destinations" homepage block Apr 13, 2018

miina requested a review from kienstra April 13, 2018 13:10

kienstra suggested changes Apr 13, 2018

View reviewed changes

miina added 2 commits April 16, 2018 15:47

Fixes from review. Improve editor style.

be159ec

Add additional destinations number check.

fe7bb55

Merge develop & resolve conflicts.

d5a5646

miina and others added 2 commits April 17, 2018 09:30

Add compiled editor JS

c133eed

kienstra approved these changes Apr 17, 2018

View reviewed changes

kienstra merged commit 8de32dd into develop Apr 17, 2018

kienstra assigned miina Apr 17, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Gutenberg] Add "Featured Destinations" homepage block #34

[Gutenberg] Add "Featured Destinations" homepage block #34

miina commented Apr 6, 2018 •

edited

Loading

miina commented Apr 6, 2018

kienstra left a comment •

edited

Loading

kienstra Apr 13, 2018

kienstra Apr 16, 2018

kienstra Apr 13, 2018

kienstra Apr 16, 2018

kienstra Apr 13, 2018

kienstra Apr 16, 2018

kienstra Apr 13, 2018

kienstra Apr 13, 2018

kienstra Apr 13, 2018

kienstra Apr 13, 2018

kienstra Apr 13, 2018 •

edited

Loading

kienstra Apr 13, 2018

kienstra Apr 13, 2018

kienstra commented Apr 14, 2018 •

edited

Loading

miina commented Apr 16, 2018

kienstra commented Apr 16, 2018 •

edited

Loading

miina commented Apr 17, 2018

kienstra commented Apr 17, 2018

miina commented Apr 18, 2018

[Gutenberg] Add "Featured Destinations" homepage block #34

[Gutenberg] Add "Featured Destinations" homepage block #34

Conversation

miina commented Apr 6, 2018 • edited Loading

miina commented Apr 6, 2018

kienstra 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kienstra Apr 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kienstra commented Apr 14, 2018 • edited Loading

miina commented Apr 16, 2018

kienstra commented Apr 16, 2018 • edited Loading

miina commented Apr 17, 2018

kienstra commented Apr 17, 2018

miina commented Apr 18, 2018

miina commented Apr 6, 2018 •

edited

Loading

kienstra left a comment •

edited

Loading

kienstra Apr 13, 2018 •

edited

Loading

kienstra commented Apr 14, 2018 •

edited

Loading

kienstra commented Apr 16, 2018 •

edited

Loading