[Gutenberg] Build a "Browse by activity" block for Homepage #30
Conversation
|
|
||
| // Replace ' with " since ' might cause issues with sanitizing. | ||
| $text = trim( str_replace( "'", '"', $text ) ); | ||
| return wp_kses( $text, $allowed_html ); |
There was a problem hiding this comment.
It looks like there are some valid <svg> elements that this doesn't allow.
For example, the "circle" SVG here. Or the shapes here.
Maybe this is intentional. If so, maybe there could be something below the <textarea> explaining what is allowed in edit_activity_meta_fields().
It looks like AMP allows several elements in svg tags, though maybe not all.
There was a problem hiding this comment.
Thanks for noting that. Originally added intentionally only the elements that were in the template but you're right -- it would probably be good to add some others. I'll add just a few others now that are used elsewhere in the template, however, perhaps it would be easier later to allow uploading an .svg file instead.
There was a problem hiding this comment.
Thanks for adding the message about allowed elements. The fact that it allows the elements in the template should be enough for now.
| <?php | ||
| // @codingStandardsIgnoreLine | ||
| echo $this->sanitize_activity_svg( $value ); | ||
| ?> |
There was a problem hiding this comment.
This isn't a blocker, but these lines could be on one line:
<?php echo $this->sanitize_activity_svg( $value ); // WPCS: XSS ok. ?>|
|
||
| $old_value = get_term_meta( $term_id, 'amp_travel_activity_svg', true ); | ||
| $new_value = isset( $_POST['travel-activity-svg'] ) ? $this->sanitize_activity_svg( $_POST['travel-activity-svg'] ) : ''; | ||
| if ( $old_value !== $new_value ) { |
There was a problem hiding this comment.
If the user enters something that is an invalid <svg> and $this->sanitize_activity_svg() strips it entirely, should this still do update_term_meta()?
There was a problem hiding this comment.
Good question, thought so initially. For example if the svg tag has illegal attributes then the svg tag will still remain, in theory it would get stripped entirely only if none of the inserted tags are allowed. But perhaps it makes more sense if the new value is saved as empty only if the unstripped version is empty (the intention was to save empty value). Will change it for now.
blocks/featured/index.js
Outdated
| /** | ||
| * Register block. | ||
| */ | ||
| export default registerBlockType( |
There was a problem hiding this comment.
This is probably beyond this PR's scope. But when we have more front-end support, maybe we could look at how to make the featured destination images expand less.
On hovering over them in the Gutenberg editor, they expand vertically much more than they do on the front-end of the ampstart site.
blocks/featured/index.js
Outdated
| { | ||
| title: __( 'Featured' ), | ||
| category: 'common', | ||
| icon: 'wordpress-alt', |
There was a problem hiding this comment.
We don't need to do this now. But it might be good to consider other icons here. Like this might be format-image', similar to the "Image" block.
There was a problem hiding this comment.
Makes sense, maybe all the blocks would need something related to the Travel theme.
blocks/travel-angles/index.js
Outdated
| // Copy from Travel HTML template. | ||
| edit() { | ||
|
|
||
| // Don't display the travel angels since that will hide the block configure buttons behind an image. |
There was a problem hiding this comment.
This should probably be "Don't display the travel angles"
| ?> | ||
| </textarea> | ||
| <p class="description"><?php esc_attr_e( 'This is for the background icon of the activity term.', 'travel' ); ?></p> | ||
| </td> |
There was a problem hiding this comment.
Could you please change this from esc_attr_e() to esc_html_e()
| * Register 'activity' and 'location' taxonomy. | ||
| */ | ||
| public function register_taxonomies() { | ||
| register_taxonomy( 'activity', array( 'adventure', 'post' ), array( |
There was a problem hiding this comment.
These values in both of the register_taxonomy() calls are the same as the default, and not needed:
- hierarchical
- public
- show_ui
|
@kienstra Thanks for reviewing, made some changed accordingly, should be ready now. |
|
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. |
Based on #12.Fixes #24.
The PR aims to:
Activity;Note that this PR already includes creatingActivitytaxonomy which is part of #27.