[Gutenberg] Add Popular homepage block. #32
Conversation
…' into feature/23-popular_block
miina
changed the title
There was a problem hiding this comment.
Good Work, A Few Points
Hi @miina,
Thanks for adding this 'Popular' homepage block.
It looks like this PR uses commits from #33, so I tried to keep the scope of the review to the diff between this PR and #33.
There will probably be more merge conflicts after merging #33.
I'm trying to merge that first, as it seems like a big dependency.
Thanks!
includes/class-amp-travel-cpt.php
Outdated
| * Adds meta boxes for adventure post type. | ||
| */ | ||
| public function add_adventure_meta_boxes() { | ||
| add_meta_box( 'amp_travel_adventure_meta', __( 'Adventure details' ), array( $this, 'adventure_meta_box_html' ), 'adventure', 'side' ); |
There was a problem hiding this comment.
Could you please add a text domain:
__( 'Adventure details', 'travel' ),
includes/class-amp-travel-cpt.php
Outdated
| */ | ||
| public function adventure_meta_box_html() { | ||
| global $post; | ||
| $adventure_custom = get_post_custom( $post->ID ); |
There was a problem hiding this comment.
How about calling get_post_custom() without an argument? It looks like that defaults to the global post ID, and it worked locally in briefly testing it.
includes/class-amp-travel-cpt.php
Outdated
| $adventure_custom = get_post_custom( $post->ID ); | ||
| $price = isset( $adventure_custom['amp_travel_price'][0] ) ? $adventure_custom['amp_travel_price'][0] : ''; | ||
| ?> | ||
| <label for='amp_travel_price'><?php esc_attr_e( 'Price (USD)', 'travel' ); ?></label> |
There was a problem hiding this comment.
Could this be esc_html_e() instead of esc_attr_e()?
includes/class-amp-travel-cpt.php
Outdated
| } | ||
|
|
||
| if ( isset( $_POST['amp_travel_price'] ) ) { | ||
| update_post_meta( $post->ID, 'amp_travel_price', esc_attr( $_POST['amp_travel_price'] ) ); |
There was a problem hiding this comment.
Could this use get_the_ID() instead of $post->ID? This works locally.
includes/class-amp-travel-cpt.php
Outdated
| * Saves the custom meta. | ||
| */ | ||
| public function save_adventure_post() { | ||
| if ( ! empty( $_POST ) ) { |
There was a problem hiding this comment.
This might not be needed. If $_POST is empty, the function will return in the next conditional when isset( $_POST['amp_travel_price'] ) is false.
includes/class-amp-travel-blocks.php
Outdated
|
|
||
| if ( ! empty( $attributes['heading'] ) ) { | ||
| $output .= '<header class="max-width-3 mx-auto px1 md-px2"> | ||
| <h3 class="h1 bold line-height-2 md-hide lg-hide" aria-hidden="true">' . esc_attr( $attributes['heading'] ) . '</h3> |
There was a problem hiding this comment.
Could this and the line below use esc_html( $attributes['heading'] ) instead of esc_attr()?
includes/class-amp-travel-blocks.php
Outdated
| </div> | ||
| </div> | ||
| <div class="h2 line-height-2 mb1"> | ||
| <span class="travel-results-result-text">' . esc_attr( get_the_title( $adventure->ID ) ) . '</span> |
There was a problem hiding this comment.
Could this please use esc_html():
esc_html( get_the_title( $adventure->ID ) )
includes/class-amp-travel-blocks.php
Outdated
| <div class="h2 line-height-2 mb1"> | ||
| <span class="travel-results-result-text">' . esc_attr( get_the_title( $adventure->ID ) ) . '</span> | ||
| <span class="travel-results-result-subtext h3">•</span> | ||
| <span class="travel-results-result-subtext h3">$ </span><span class="black bold">' . esc_attr( $price ) . '</span> |
There was a problem hiding this comment.
Could this please use esc_html():
esc_html( $price )
includes/class-amp-travel-blocks.php
Outdated
|
|
||
| $output .= '</div> | ||
| </div> | ||
| <span class="travel-results-result-subtext mr1">' . esc_attr( $comments->approved ) . esc_attr__( ' Reviews' ) . '</span> |
There was a problem hiding this comment.
Could this be something like:
sprintf( esc_html__( '%d Reviews', 'travel' ), esc_html( $comments->approved ) )
With a translators comment above:
<?php /* translators: %d: The number of reviews */ ?>
includes/class-amp-travel-blocks.php
Outdated
| </div> | ||
| <span class="travel-results-result-subtext mr1">' . esc_attr( $comments->approved ) . esc_attr__( ' Reviews' ) . '</span> | ||
| <span class="travel-results-result-subtext"><svg class="travel-icon" viewBox="0 0 77 100"><g fill="none" fillRule="evenodd"><path stroke="currentColor" strokeWidth="7.5" d="M38.794 93.248C58.264 67.825 68 49.692 68 38.848 68 22.365 54.57 9 38 9S8 22.364 8 38.85c0 10.842 9.735 28.975 29.206 54.398a1 1 0 0 0 1.588 0z"></path><circle cx="38" cy="39" r="10" fill="currentColor"></circle></g></svg> | ||
| ' . esc_attr( $location ) . '</span> |
There was a problem hiding this comment.
Could you please change this to esc_html( $location )
|
@kienstra Thanks for reviewing, made the changes + some other small changes found when testing the block. |
|
Note that the heading of the block is static, looks like the texts of dynamic blocks can't be edited directly as with other blocks (at least with the Gutenberg stable version as of current moment) and making the heading editable is something to look into later. |
This was the only remaining issue in the PR. So I made this change on my own to merge it.
There were conflicts in: assets/css/editor-blocks.css includes/class-amp-travel-blocks.php Retain edits from both develop and this branch: feature/23-popular_block Also, change order of register_block_travel_popular() and render_block_travel_popular() But make no change in those methods.
These look to have been from PR #33. And they might be valid. But defer the discussion of those to that PR.
There were conflics in: assets/css/editor-blocks.css includes/class-amp-travel-theme.php In AMP_Travel_Theme, these were simply from reorganizing that class in a recent other PR.
|
Merging With #33 merged into |
Based on #12.Fixes #23.
Intends to:
rating;adventurepost data dynamically;This PR is blocked by #7.