Adds dynamic header and footer in WordPress theme. #36

kopepasah · 2018-01-19T22:40:16Z

Integrates dynamic header and footer from static templates.

Covers #33

ThierryA

Great work @kopepasah. I left my CR below.

ThierryA · 2018-01-22T13:59:01Z

inc/template-functions.php

+	}
+
+	$form = '<li class="menu-item menu-item-search-form">
+		<form role="search" method="get" target="_top" action="' . esc_url( home_url( '/' ) ) . '">


Use get_search_form(). Let me know if it doesn't validate.

@ThierryA the reason I did not use get_search_form() here is the default search for does not have the correct markup. We could create a search-form.php template with the markup required for this search form, but then that would apply to all other instances of get_search_form() as well.

@kopepasah I think you should go ahead and use get_search_form(). The form sanitizer in ampproject/amp-wp#871 will automatically add target="_top" to the form here. Otherwise, we could still use get_search_form() and then use the get_search_form filter to inject the target attribute onto it.

@westonruter the reason for this markup is not only to add the target attribute, it was for stylistic purposes. However, if it is required, I can rework it to use get_search_form().

@kopepasah if just for stylistic purposes then I think it would be good to create a search-form.php and then also use get_search_form(), like you suggested. We'd be making sure that the AMP plugin forces the default output of get_search_form() to be valid AMP, so it would be a good pattern to use for other themers to copy.

I ended up creating a custom template and used get_search_form().

ThierryA · 2018-01-22T14:00:13Z

inc/template-functions.php

+ *
+ * @return string $items
+ */
+function ampconf_wp_nav_menu_items( $items, $args ) {


Could we perhaps have the search form after the nav ul. If it doesn't call to much styling changes, we can then add it to the header template.

@ThierryA it definitely takes less styles to hook it into the header menu (not loads less, but less). Since we are not limited on HTML, but are limited on styles, I think this implementation is a little better for this use case.

@kopepasah it actually doesn't require more styling 😄 (see the latest changes I pushed
). Is quite beneficial as we can call the search form straight in the header.php and avoid having the hook firing for all menus.

ThierryA · 2018-01-22T14:00:48Z

inc/template-functions.php

@@ -30,3 +30,31 @@ function ampconf_pingback_header() {
 	}
 }
 add_action( 'wp_head', 'ampconf_pingback_header' );
+
+/**
+ * Adds the search for as the last top-level menu item.


I believe a word is missing here.

ThierryA · 2018-01-22T14:04:36Z

functions.php

+	return $amp_scripts;
+}
+add_filter( 'amp_component_scripts', 'ampconf_amp_component_scripts' );
+
 /**
 * Enqueue scripts and styles.
 */
 function ampconf_scripts() {


We can get rid of this function since assets are dequeued by the AMP plugin. (We will add the necessary fallback for .org when we get to that story)

This is actually not accurate. Assets don't get dequeued, but rather the printing is prevented. And with ampproject/amp-wp#887 actually the enqueued assets are used for printing.

ThierryA · 2018-01-22T14:05:17Z

functions.php

@@ -129,12 +132,40 @@ function ampconf_widgets_init() {
 }
 add_action( 'widgets_init', 'ampconf_widgets_init' );

+/**
+ * Filters the AMP custom css to include the CSS from out theme.


Typo mistake.

ThierryA · 2018-01-22T14:07:21Z

functions.php

+ * @return array
+ */
+function ampconf_amp_component_scripts( $amp_scripts ) {
+	$amp_scripts['amp-form'] = 'https://cdn.ampproject.org/v0/amp-form-0.1.js';


This should be automatically be included by the plugin once the document sanitization is completed. Since it isn't the case at the moment, add // @todo as a reminder to remove it.

ThierryA · 2018-01-22T14:15:10Z

footer.php

-		</div><!-- .site-info -->
-	</footer><!-- #colophon -->
-</div><!-- #page -->
+	<p class="wrap__item wrap__item--footer-copyright">&copy; <?php echo esc_html( date( 'Y' ) ); ?> <a href="<?php echo esc_url( home_url( '/' ) ); ?>" rel="home"><?php bloginfo( 'name' ); ?></a></p>


I think © is universal but I think we should change it to <?php printf( '© %s', esc_html( date( 'Y' ) ) ); ?> just to be safe.

westonruter · 2018-01-22T19:39:54Z

functions.php

@@ -44,10 +44,13 @@ function ampconf_setup() {
 		 */
 		add_theme_support( 'post-thumbnails' );

-		// This theme uses wp_nav_menu() in one location.
+		/***


Trivial but /** is reserved for phpdoc, like for functions, methods, variable definitions, etc. For other cases, just use:

/* ... */

But since this is a single-line comment anyway, then // I think is right.

Comment changed.

westonruter · 2018-01-22T19:40:43Z

functions.php

+
+	return $styles . $css;
+}
+add_filter( 'amp_custom_styles', 'ampconf_custom_styles' );


This can be eliminated in favor of regular enqueueing with ampproject/amp-wp#887

Styles are now enqueued the standard method.

kopepasah · 2018-01-22T19:43:04Z

@ThierryA adjusted based on your review and should be ready to go.

# Conflicts: # assets/dist/css/main.css

Dependent on ampproject/amp-wp#887 before working.

kopepasah added 9 commits January 19, 2018 16:34

Issue 33 Add WordPress menus.

1c771d5

Issue 33 Filter custom styles and removes unnecessary styles.

f5302b0

33 Add custom component scripts.

7b9d415

33 Add dynamic header markup.

6f30f4c

33 Only add search form in header menu.

f83df11

33 Add dynamic site footer.

bf86b63

33 Fix 🐛, header centered alignment.

a9f3f03

33 Fix 🐛, improve search input text.

b219f8d

33 Fix , header font size and margin.

61b15ec

kopepasah changed the title ~~WIP - Add/33~~ WIP - Adds dynamic header and footer in WordPress theme. Jan 21, 2018

kopepasah requested a review from ThierryA January 21, 2018 23:47

kopepasah added the backend label Jan 21, 2018

33 Build Assets.

a0ddd2c

kopepasah changed the title ~~WIP - Adds dynamic header and footer in WordPress theme.~~ Adds dynamic header and footer in WordPress theme. Jan 21, 2018

ThierryA approved these changes Jan 22, 2018

View reviewed changes

kopepasah added 4 commits January 22, 2018 12:56

33 Print copyright.

efaf106

33 Fix typo and adjust comment.

f50c801

33 Add @todo as a reminder this will be removed.

66c2405

33 Remove unused enqueue scripts function.

0416daf

westonruter reviewed Jan 22, 2018

View reviewed changes

33 Adjust comment for menu hook.

f56cac2

kopepasah and others added 7 commits January 22, 2018 13:44

Merge branch 'develop' into add/33

d3bdd34

# Conflicts: # assets/dist/css/main.css

33 Edit nav menu comment.

3f6f1cd

33 Implement custom search form template.

82de045

33 Enqueue styles the standard WordPress way.

b83a900

Dependent on ampproject/amp-wp#887 before working.

Fix style sheet path

2458e1f

Move form outside the menu

efd1f6f

Use WP default translation where possible

bbcb3a5

ThierryA added 3 commits January 23, 2018 12:02

Improve header styling

327d88b

Adjusted static file header according to the latest BE changes

9e0045e

Merge branch 'develop' into add/33

5dd214d

ThierryA requested a review from delawski January 23, 2018 11:18

delawski approved these changes Jan 23, 2018

View reviewed changes

ThierryA merged commit 33e83b2 into develop Jan 23, 2018

ThierryA deleted the add/33 branch January 23, 2018 11:20

ThierryA restored the add/33 branch January 23, 2018 11:26

kopepasah mentioned this pull request Jan 23, 2018

Integrate static header and footer into WordPress theme. #33

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds dynamic header and footer in WordPress theme. #36

Adds dynamic header and footer in WordPress theme. #36

kopepasah commented Jan 19, 2018 •

edited

Loading

ThierryA left a comment

ThierryA Jan 22, 2018

kopepasah Jan 22, 2018

westonruter Jan 22, 2018

kopepasah Jan 22, 2018

westonruter Jan 22, 2018

kopepasah Jan 23, 2018

ThierryA Jan 22, 2018

kopepasah Jan 22, 2018

ThierryA Jan 23, 2018 •

edited

Loading

ThierryA Jan 22, 2018

ThierryA Jan 22, 2018

westonruter Jan 22, 2018

ThierryA Jan 22, 2018

ThierryA Jan 22, 2018

ThierryA Jan 22, 2018

westonruter Jan 22, 2018

kopepasah Jan 23, 2018

westonruter Jan 22, 2018

kopepasah Jan 23, 2018

kopepasah commented Jan 22, 2018

Adds dynamic header and footer in WordPress theme. #36

Adds dynamic header and footer in WordPress theme. #36

Conversation

kopepasah commented Jan 19, 2018 • edited Loading

ThierryA left a 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

ThierryA Jan 23, 2018 • 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

kopepasah commented Jan 22, 2018

kopepasah commented Jan 19, 2018 •

edited

Loading

ThierryA Jan 23, 2018 •

edited

Loading