Skip to content
This repository has been archived by the owner on Oct 10, 2018. It is now read-only.

Adds dynamic header and footer in WordPress theme. #36

Merged
merged 25 commits into from
Jan 23, 2018
Merged

Adds dynamic header and footer in WordPress theme. #36

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
1c771d5
Issue 33 Add WordPress menus.
kopepasah Jan 19, 2018
f5302b0
Issue 33 Filter custom styles and removes unnecessary styles.
kopepasah Jan 19, 2018
7b9d415
33 Add custom component scripts.
kopepasah Jan 21, 2018
6f30f4c
33 Add dynamic header markup.
kopepasah Jan 21, 2018
f83df11
33 Only add search form in header menu.
kopepasah Jan 21, 2018
bf86b63
33 Add dynamic site footer.
kopepasah Jan 21, 2018
a9f3f03
33 Fix 🐛, header centered alignment.
kopepasah Jan 21, 2018
b219f8d
33 Fix 🐛, improve search input text.
kopepasah Jan 21, 2018
61b15ec
33 Fix , header font size and margin.
kopepasah Jan 21, 2018
a0ddd2c
33 Build Assets.
kopepasah Jan 21, 2018
efaf106
33 Print copyright.
kopepasah Jan 22, 2018
f50c801
33 Fix typo and adjust comment.
kopepasah Jan 22, 2018
66c2405
33 Add @todo as a reminder this will be removed.
kopepasah Jan 22, 2018
0416daf
33 Remove unused enqueue scripts function.
kopepasah Jan 22, 2018
f56cac2
33 Adjust comment for menu hook.
kopepasah Jan 22, 2018
d3bdd34
Merge branch 'develop' into add/33
kopepasah Jan 22, 2018
3f6f1cd
33 Edit nav menu comment.
kopepasah Jan 23, 2018
82de045
33 Implement custom search form template.
kopepasah Jan 23, 2018
b83a900
33 Enqueue styles the standard WordPress way.
kopepasah Jan 23, 2018
2458e1f
Fix style sheet path
ThierryA Jan 23, 2018
efd1f6f
Move form outside the menu
ThierryA Jan 23, 2018
bbcb3a5
Use WP default translation where possible
ThierryA Jan 23, 2018
327d88b
Improve header styling
ThierryA Jan 23, 2018
9e0045e
Adjusted static file header according to the latest BE changes
ThierryA Jan 23, 2018
5dd214d
Merge branch 'develop' into add/33
ThierryA Jan 23, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion assets/dist/css/main.css
View file
Open in desktop

Large diffs are not rendered by default.

8 changes: 8 additions & 0 deletions assets/src/css/modules/_header.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,8 @@

&--header &__item--branding {
position: relative;
margin-left: auto;
margin-right: auto;

@media ( query( max, tablet ) ) {
background: $black;
Expand All @@ -61,6 +63,9 @@
}

&-title {
font-size: 56px;
font-weight: 700;
margin: 32px 0 0;

a {
color: $white;
Expand Down Expand Up @@ -180,9 +185,12 @@
font-size: 14px;
max-width: 70px;
transition: max-width 100ms linear, background-color 100ms linear;
color: $white;
text-overflow: ellipsis;

&:focus {
background-color: $white;
color: $black;
max-width: 200px;
}

Expand Down
33 changes: 14 additions & 19 deletions footer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,33 +2,28 @@
/**
* The template for displaying the footer
*
* Contains the closing of the #content div and all content after.
*
* @link https://developer.wordpress.org/themes/basics/template-files/#template-partials
*
* @package AMPConf
*/

?>

</div><!-- #content -->
<footer class="wrap wrap--full-width wrap--footer">
<nav class="wrap__item wrap__item--footer-menu">
<?php
wp_nav_menu(
array(
'theme_location' => 'footer',
'menu_id' => false,
'depth' => 1,
)
);
?>
</nav>

<footer id="colophon" class="site-footer">
<div class="site-info">
<a href="<?php echo esc_url( __( 'https://wordpress.org/', 'ampconf' ) ); ?>">
<?php
/* translators: %s: CMS name, i.e. WordPress. */
printf( esc_html__( 'Proudly powered by %s', 'ampconf' ), 'WordPress' );
?>
</a>
<span class="sep"> | </span>
<?php
/* translators: 1: Theme name, 2: Theme author. */
printf( esc_html__( 'Theme: %1$s by %2$s.', 'ampconf' ), 'ampconf', '<a href="http://underscores.me/">Me</a>' );
?>
</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>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

</footer>

<?php wp_footer(); ?>

Expand Down
39 changes: 35 additions & 4 deletions functions.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,10 +44,13 @@ function ampconf_setup() {
*/
add_theme_support( 'post-thumbnails' );

// This theme uses wp_nav_menu() in one location.
/***
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment changed.

* Registers multiple menus
*/
register_nav_menus(
array(
'menu-1' => esc_html__( 'Primary', 'ampconf' ),
'header' => esc_html__( 'Header', 'ampconf' ),
'footer' => esc_html__( 'Footer', 'ampconf' ),
)
);

Expand Down Expand Up @@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo mistake.

*
* @param string $css The combined custom css.
*
* @return string $css
*/
function ampconf_custom_styles( $css ) {
$path = get_template_directory() . '/assets/dist/css/main.css';
$styles = file_get_contents( $path ); // phpcs:ignore WordPress.WP.AlternativeFunctions -- Not a remote file.

return $styles . $css;
}
add_filter( 'amp_custom_styles', 'ampconf_custom_styles' );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Styles are now enqueued the standard method.


/**
* Adds custom component scripts to the document.
*
* @param array $amp_scripts AMP Component scripts, mapping component names to component source URLs.
*
* @return array
*/
function ampconf_amp_component_scripts( $amp_scripts ) {
$amp_scripts['amp-form'] = 'https://cdn.ampproject.org/v0/amp-form-0.1.js';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

$amp_scripts['amp-bind'] = 'https://cdn.ampproject.org/v0/amp-bind-0.1.js';

return $amp_scripts;
}
add_filter( 'amp_component_scripts', 'ampconf_amp_component_scripts' );

/**
* Enqueue scripts and styles.
*/
function ampconf_scripts() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

wp_enqueue_style( 'ampconf-style', get_stylesheet_uri() );

if ( is_singular() && comments_open() && get_option( 'thread_comments' ) ) {
wp_enqueue_script( 'comment-reply' );
}
Expand Down
39 changes: 20 additions & 19 deletions header.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,35 +18,36 @@
</head>

<body <?php body_class(); ?>>
<div id="page" class="site">
<a class="skip-link screen-reader-text" href="#content"><?php esc_html_e( 'Skip to content', 'ampconf' ); ?></a>

<header id="masthead" class="site-header">
<div class="site-branding">
<?php the_custom_logo(); ?>
<?php if ( is_front_page() && is_home() ) : ?>
<h1 class="site-title"><a href="<?php echo esc_url( home_url( '/' ) ); ?>" rel="home"><?php bloginfo( 'name' ); ?></a></h1>
<?php else : ?>
<p class="site-title"><a href="<?php echo esc_url( home_url( '/' ) ); ?>" rel="home"><?php bloginfo( 'name' ); ?></a></p>
<?php endif; ?>
<header class="wrap wrap--header wrap--header__menu-hidden" [class]="mobileMenu ? 'wrap wrap--header' : 'wrap wrap--header wrap--header__menu-hidden'">

<div class="wrap__item wrap__item--branding">
<<?php ampconf_branding_tag(); ?> class="wrap__item--branding-title">
<?php if ( has_custom_logo() ) : ?>
<?php the_custom_logo(); ?>
<?php else : ?>
<a href="<?php echo esc_url( home_url( '/' ) ); ?>" rel="home"><?php bloginfo( 'name' ); ?></a>
<?php endif; ?>

</<?php ampconf_branding_tag(); ?>>

<?php $description = get_bloginfo( 'description', 'display' ); ?>
<?php if ( $description || is_customize_preview() ) : ?>
<p class="site-description"><?php echo $description; /* WPCS: xss ok. */ ?></p>
<p class="wrap__item--branding-description"><?php echo $description; /* WPCS: xss ok. */ ?></p>
<?php endif; ?>
</div><!-- .site-branding -->

<nav id="site-navigation" class="main-navigation">
<button class="menu-toggle" aria-controls="primary-menu" aria-expanded="false"><?php esc_html_e( 'Primary Menu', 'ampconf' ); ?></button>
<button class="wrap__item--menu-toggle" on="tap:AMP.setState( { mobileMenu: ! mobileMenu } )" aria-controls="primary-menu" aria-expanded="false"></button>
</div>

<nav class="wrap__item wrap__item--menu">
<?php
wp_nav_menu(
array(
'theme_location' => 'menu-1',
'menu_id' => 'primary-menu',
'theme_location' => 'header',
'menu_id' => false,
)
);
?>
</nav><!-- #site-navigation -->
</header><!-- #masthead -->

<div id="content" class="site-content">
</nav>
</header>
28 changes: 28 additions & 0 deletions inc/template-functions.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe a word is missing here.

*
* @param string $items The HTML list content for the menu items.
* @param stdClass $args An object containing wp_nav_menu() arguments.
*
* @return string $items
*/
function ampconf_wp_nav_menu_items( $items, $args ) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor

@ThierryA ThierryA Jan 23, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

if ( 'header' !== $args->theme_location ) {
return $items;
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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().

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

<label>
<span class="screen-reader-text">' . _x( 'Search for:', 'label', 'ampconf' ) . '</span>
<input type="search" placeholder="' . esc_attr_x( 'Search', 'placeholder', 'ampconf' ) . '" value="' . get_search_query() . '" name="s" />
</label>

<button type="submit"></button>
</form>
</li>';

return $items . $form;
}
add_filter( 'wp_nav_menu_items', 'ampconf_wp_nav_menu_items', 10, 2 );
9 changes: 9 additions & 0 deletions inc/template-tags.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,3 +142,12 @@ function ampconf_post_thumbnail() {
endif; // End is_singular().
}
endif;

if ( ! function_exists( 'ampconf_branding_tag' ) ) :
/**
* Outputs the branding in header as h1 or p, depending on the current view.
*/
function ampconf_branding_tag() {
echo esc_html( ( is_front_page() && is_home() ) ? 'h1' : 'p' );
}
endif;
Loading