36 - Single product template #39

mehigh · 2018-04-15T17:11:10Z

This is left:

Top carousel
mobile fixed CTA overlay
mobile content product details / price size / inlining

And of course copying the product.css from ampstart (i've setup that repo next to this and referenced the CSS with a regular CSS link during development).
Reached 44KB of CSS until now.

miina · 2018-04-16T20:01:14Z

Started adding theme integration
Currently (within 9d60672) using placeholders for header.php, footer.php, and static comments.php from @mehigh's created HTML template.

Maybe comments.php can be implemented within this PR since that's directly related to the CPT template but perhaps proper header template(s) and footer template could be separate PR-s/issues? Thoughts, @kienstra and @postphotos?

kienstra · 2018-04-17T03:37:39Z

Header And Footer Templates

Hi @miina,

...perhaps proper header template(s) and footer template could be separate PR-s/issues

That's a good idea. Separate issues and PRs for header.php and footer.php would help organize this.

And we could merge the templates into develop as soon as they're ready, instead of waiting for all of them.

DavidCramer · 2018-04-17T10:31:44Z

@miina - please take a look at #38 as it could affect the structures.

…ult.

miina · 2018-04-17T16:58:47Z

@kienstra Forgot to mention that this PR also fixes #8 (Ratings logic).

kienstra · 2018-04-17T17:22:54Z

templates/footer-search.php

+
+?>
+<!-- Search -->
+<section class='travel-search py4 xs-hide sm-hide relative'>


This isn't a blocker, but it'd be nice to have double quotes in the template. We can go back and address this after the demo on Thursday.

kienstra · 2018-04-17T17:26:38Z

templates/entry/meta/reviews-count.php

+$reviews = wp_count_comments( get_the_ID() );
+?>
+<?php /* translators: %d: Number of reviews. */ ?>
+<p class="travel-results-result-subtext"><?php echo sprintf( esc_html__( '%d Reviews', 'travel' ), esc_html( $reviews->approved ) ); ?></p>


This could be printf() instead of echo sprintf().

kienstra · 2018-04-17T17:34:44Z

templates/entry/meta/rating.php

+?>
+<div class="relative h2 line-height-2 mb1">
+	<div class="travel-results-result-stars green">
+		<?php for ( $i = 0; $i < $rating; $i++ ) : ?>


How about something like:

echo esc_html( str_repeat( '*', $rating ) );

kienstra · 2018-04-17T17:36:36Z

templates/footer-search.php

+<!-- Search -->
+<section class='travel-search py4 xs-hide sm-hide relative'>
+	<div class='px1 md-px2 pb1 relative'>
+		<h3 class='travel-search-heading travel-spacing-none h1 bold mb2 center'><?php esc_html_e( 'Have a specific destination in mind?' ); ?></h3>


Could this include the text domain of 'travel', and likewise for the other translation functions in the file?

esc_html_e( 'Have a specific destination in mind?', 'travel' )

kienstra · 2018-04-17T17:40:56Z

templates/entry/full.php

+			<option value="1"><?php esc_html_e( 'For 1' ); ?></option>
+			<?php for ( $i = 2; $i <= 8; $i += 2 ) : ?>
+				<?php /* translators: %d: The number of people */ ?>
+				<option value="<?php echo esc_attr( $i ); ?>"><?php echo sprintf( esc_html__( 'For %d', 'travel' ), esc_html( $i ) ); ?></option>


This is a minor point, but this could be printf instead of echo sprintf.

kienstra · 2018-04-17T17:41:39Z

templates/entry/full.php

+			<?php endfor; ?>
+		</select>
+
+		<a href="#" class="product-cta-btn ampstart-btn rounded center bold white block col-12"><?php esc_html_e( 'Call to book' ); ?></a>


Could this please use the text domain of 'travel'?

kienstra · 2018-04-17T17:43:27Z

templates/similar-adventures.php

+
+?>
+<!-- Popular -->
+<?php echo amp_travel_render_similar_adventures(); // WPCS: XSS ok. ?>


Thanks for creating this template tag.

kienstra · 2018-04-17T17:55:03Z

footer.php

+ * @package WPAMPTheme
+ */
+
+// Ignore the issues of this file since that's just a copied HTML placeholder.


Though this footer.php is only a placeholder, it looks good. We could work on dynamic data later, at least after the screencast this Thursday.

kienstra · 2018-04-17T18:10:19Z

includes/functions.php

+			<span class="travel-results-result-subtext mr1">' .
+			/* translators: %d: The number of reviews */
+			sprintf( esc_html__( '%d Reviews', 'travel' ), esc_html( $comments->approved ) ) . '</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>


The plugin's validator raised invalid_attribute errors forfillRule and strokeWidth.

Similar errors appear on copying this entire <svg> into the AMP validator:

The attribute 'fillrule' may not appear in tag 'g'.
The attribute 'strokewidth' may not appear in tag 'path'

The page is still valid AMP, but the preprocessor stripped those attributes.

kienstra · 2018-04-17T18:11:33Z

templates/entry/meta/location.php

+$location  = ! empty( $locations ) ? $locations[0] : '--';
+?>
+<p class="travel-results-result-subtext mb1">
+	<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>


As mentioned, it looks like fillrule and strokewidth aren't allowed. The plugin's preprocessor removes them, so the page is still valid AMP. But just so we're aware that they're not appearing on the front-end.

kienstra · 2018-04-17T19:11:50Z

comments.php

+if ( post_password_required() ) {
+	return;
+}
+


This is outside this PR's diff, but could you please add 'comments' to the 'adventure' CPT 'supports' argument? I think this is blocking the "Popular Adventures" block from displaying, as you can't add ratings (comments) to the 'adventure' post type.

kienstra · 2018-04-17T19:19:33Z

includes/functions.php

+			<option value="">--</option>';
+
+	for ( $i = 5; $i >= 1; $i-- ) {
+		echo '<option value="' . esc_attr( $i ) . '">' . sprintf( '%d star(s)', esc_attr( $i ) ) . '</option>';


This is looking good. What do you think of this small change to separate the singular and plural?

Instead of

'%d star(s)'

how about:

_n( '%d star', '%d stars', $i, 'travel' )

kienstra · 2018-04-17T19:32:12Z

templates/entry/full.php

+
+	<div class="product-cta lg-col-2 lg-right-align">
+		<h4 class="product-cta-title mb2 line-height-1" [text]="'$' + adventure_current_price">$<?php echo esc_html( get_post_meta( get_the_ID(), 'amp_travel_price', true ) ); ?></h4>
+		<select class="select-arr mb1 rounded" name="count" on="change:AMP.setState({ adventure_current_price: event.value * adventure_price })">


Great job with updating the price based on the selection.

The develop branch deletes that file, so having changes in that file in this PR caused merge conflics. Please do npm run build when checking this out.

kienstra · 2018-04-18T04:26:10Z

Building editor-blocks.js

The develop branch removes editor-blocks.js, in order to avoid merge conflicts in PRs. So I also removed that file from this branch to remove the conflict to develop.

Please do npm run build when checking out this branch to use the Gutenberg blocks.

mehigh · 2018-04-18T08:38:55Z

The html/css is complete for the page now:

…nto feature/36-adventure-single

miina · 2018-04-18T11:09:41Z

includes/functions.php

+
+	for ( $i = 5; $i >= 1; $i-- ) {
+		/* translators: %d: Rating */
+		echo '<option value="' . esc_attr( $i ) . '">' . sprintf( _n( '%d star', '%d stars', $i, 'travel' ), number_format_i18n( $i ) ) . '</option>';


@kienstra This line is now throwing CS error:
includes/functions.php:153:67: error - All output should be run through an escaping function (see the Security sections in the WordPress Developer Handbooks), found '_n'. (WordPress.XSS.EscapeOutput.OutputNotEscaped).

Any thoughts how to fix that?

Hi @miina, wrapping the _n() call in esc_html() should take care of that. Otherwise:

// WPCS: XSS ok.

miina · 2018-04-18T11:11:00Z

@kienstra Thanks for reviewing, most of the issues should be addressed now. Left the gallery as static for now, hopefully that's OK for now and could be addressed later if needed. There's just one issue that I'm not sure how to fix, any thoughts?

@mehigh Thanks for generating the HTML!
There's one CSS issue that would be great if you could help with -- namely within the Similar Posts section there's a div with class travel-overflow-container. The CSS for the class in the adventure.css looks like this:

.travel-overflow-container {
    display: inline-block;
    min-width: 100vw;
    padding-left: calc((100vw - 72rem) / 2);
    padding-right: calc((100vw - 72rem) / 2);
}

However, with the AMP plugin on it looks like the padding gets stripped and only this is left:

.travel-overflow-container {
    display: inline-block;
    min-width: 100vw;
}

Would be great if you could help with that issue.

…nto feature/36-adventure-single

It may be that number_format_i18n() needs esc_html(). WordPress.XSS.EscapeOutput.OutputNotEscaped

…nto feature/36-adventure-single

kienstra

Approved

Hi @miina and @mehigh,
Thanks for your great work here. The single 'adventure' template looks good.

postphotos · 2018-04-18T15:25:44Z

Making sure we capture - related to #36.

36 - Single product HTML

e4f82c1

mehigh assigned miina Apr 15, 2018

miina added 3 commits April 16, 2018 19:13

Start adding theme implementation for single-adventure.php

9d60672

Fix phpcs.

dcb8d4b

Fix CS.

4157d49

Add method comment.

a1f0c89

Merge develop & resolve conflicts.

2b9645a

miina added 2 commits April 17, 2018 16:27

Add functionality for comments. Refactor templates.

e2ea438

Add nonce based on default comments saving. Show rating empty by defa…

ae3ac4d

…ult.

miina mentioned this pull request Apr 17, 2018

As a Travel website user, I need a rating to understand review scores on a given post #8

Open

Add ampState for price.

c5efef8

kienstra reviewed Apr 17, 2018

View reviewed changes

kienstra mentioned this pull request Apr 17, 2018

[WIP] Implement PSTT theme #38

Closed

4 tasks

Merge in develop, delete editor-blocks.js.

0d6065f

The develop branch deletes that file, so having changes in that file in this PR caused merge conflics. Please do npm run build when checking this out.

kienstra mentioned this pull request Apr 18, 2018

[Don't Merge] PR #39 commits, pushed to a branch that can deploy to a Pantheon multidev environment #41

Closed

mehigh added 5 commits April 18, 2018 09:58

36 - top hero carousel

fced3dc

36 - Product details adjustments for responsiveness

2e29d94

36 - Sticky call to action

b656c60

36 - updated CSS for single product

7877b24

36 - copied images from ampstart travel template

338e9a6

mehigh and others added 7 commits April 18, 2018 11:47

36 - fixed validation error

820e9d5

Template updates.

6c468a9

Merge branch 'feature/36-adventure-single' of github.com:xwp/travel i…

fac3f4a

…nto feature/36-adventure-single

Add sticky CTA.

f3a1fcb

Fixes from review.

57868cf

Fix popular posts.

6057eba

Fix popular posts.

0c4e472

miina reviewed Apr 18, 2018

View reviewed changes

Fix phpcs.

98ec3ee

kienstra self-assigned this Apr 18, 2018

mehigh and others added 6 commits April 18, 2018 16:26

36 - Added a class to work around the calc

6194fd3

36 - added workaround to failing calc styles

808d91f

Merge branch 'feature/36-adventure-single' of github.com:xwp/travel i…

3ea4b0a

…nto feature/36-adventure-single

Add esc_html to address Travis issue.

6c9617d

It may be that number_format_i18n() needs esc_html(). WordPress.XSS.EscapeOutput.OutputNotEscaped

Style fixes.

bc3970c

Merge branch 'feature/36-adventure-single' of github.com:xwp/travel i…

b945550

…nto feature/36-adventure-single

miina changed the title ~~[WIP] 36 - Single product template~~ 36 - Single product template Apr 18, 2018

kienstra approved these changes Apr 18, 2018

View reviewed changes

kienstra merged commit 6848c6e into develop Apr 18, 2018

kienstra deleted the feature/36-adventure-single branch April 18, 2018 14:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

36 - Single product template #39

36 - Single product template #39

mehigh commented Apr 15, 2018

miina commented Apr 16, 2018

kienstra commented Apr 17, 2018 •

edited

Loading

DavidCramer commented Apr 17, 2018

miina commented Apr 17, 2018

kienstra Apr 17, 2018

kienstra Apr 17, 2018

kienstra Apr 17, 2018

kienstra Apr 17, 2018

kienstra Apr 17, 2018

kienstra Apr 17, 2018

kienstra Apr 17, 2018

kienstra Apr 17, 2018 •

edited

Loading

kienstra Apr 17, 2018 •

edited

Loading

kienstra Apr 17, 2018 •

edited

Loading

kienstra Apr 17, 2018 •

edited

Loading

kienstra Apr 17, 2018 •

edited

Loading

kienstra Apr 17, 2018 •

edited

Loading

kienstra commented Apr 18, 2018 •

edited

Loading

mehigh commented Apr 18, 2018

miina Apr 18, 2018

kienstra Apr 18, 2018 •

edited

Loading

miina commented Apr 18, 2018

kienstra left a comment

postphotos commented Apr 18, 2018

36 - Single product template #39

36 - Single product template #39

Conversation

mehigh commented Apr 15, 2018

miina commented Apr 16, 2018

kienstra commented Apr 17, 2018 • edited Loading

DavidCramer commented Apr 17, 2018

miina commented Apr 17, 2018

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 17, 2018 • edited Loading

Choose a reason for hiding this comment

kienstra Apr 17, 2018 • edited Loading

Choose a reason for hiding this comment

kienstra Apr 17, 2018 • edited Loading

Choose a reason for hiding this comment

kienstra Apr 17, 2018 • edited Loading

Choose a reason for hiding this comment

kienstra Apr 17, 2018 • edited Loading

Choose a reason for hiding this comment

kienstra Apr 17, 2018 • edited Loading

Choose a reason for hiding this comment

kienstra commented Apr 18, 2018 • edited Loading

mehigh commented Apr 18, 2018

Choose a reason for hiding this comment

kienstra Apr 18, 2018 • edited Loading

Choose a reason for hiding this comment

miina commented Apr 18, 2018

kienstra left a comment

Choose a reason for hiding this comment

postphotos commented Apr 18, 2018

kienstra commented Apr 17, 2018 •

edited

Loading

kienstra Apr 17, 2018 •

edited

Loading

kienstra Apr 17, 2018 •

edited

Loading

kienstra Apr 17, 2018 •

edited

Loading

kienstra Apr 17, 2018 •

edited

Loading

kienstra Apr 17, 2018 •

edited

Loading

kienstra Apr 17, 2018 •

edited

Loading

kienstra commented Apr 18, 2018 •

edited

Loading

kienstra Apr 18, 2018 •

edited

Loading