WordPress.org

Make WordPress Core

Opened 9 months ago

Closed 9 months ago

Last modified 9 months ago

#38927 closed defect (bug) (fixed)

Twenty Seventeen: site navigation to position:fixed too early on front page without header image

Reported by: LittleBigThing Owned by: davidakennedy
Milestone: 4.7 Priority: normal
Severity: normal Version: 4.7
Component: Bundled Theme Keywords: has-patch commit dev-reviewed
Focuses: ui, template Cc:

Description

Twenty Seventeen uses position: fixed for the main-navigation when the header image/video is scrolled by on larger screens. The fixed position is set by adding .site-navigation-fixed to the div .navigation-top using JS. All working smooth, except on the front/blog page when no header image/video is set.

The class site-navigation-fixedis set based on the offset of the navigation (headerOffset) in assets/js/global.js:

if ( isFrontPage && $customHeaderImage.length ) {
	headerOffset = $customHeader.innerHeight() - navigationOuterHeight;
} else {
	headerOffset = $customHeader.innerHeight();
}

$customHeaderImage.lengthis never 0 since there is always a div.custom-header-image, even if there is no custom header is set (see template-parts/header/header-image.php).

IMHO, the cleanest fix would be to avoid having a div.custum-header-image when there is no header image/video set. This is achieved by adding a check in template-parts/header/header-image.php before outputting div.custum-header-image:

<?php if ( has_header_image() || has_header_video() ) : ?>
	<div class="custom-header-image">
		<?php the_custom_header_markup(); ?>
	</div>
<?php endif; ?>

Another option would be to check whether div.custum-header-image is empty based on its html content (including some trimming): $.trim($customHeaderImage.html()).

So something like this in assets/js/global.js:

if ( isFrontPage && $.trim($customHeaderImage.html()) ) {
	headerOffset = $customHeader.innerHeight() - navigationOuterHeight;
} else {
	headerOffset = $customHeader.innerHeight();
}

Attachments (1)

38927.patch (709 bytes) - added by laurelfulford 9 months ago.

Download all attachments as: .zip

Change History (12)

#1 @LittleBigThing
9 months ago

The check above whether to output the div should probably better use 'has_custom_header()'. Just read about it... :-)

Last edited 9 months ago by LittleBigThing (previous) (diff)

This ticket was mentioned in Slack in #core-themes by davidakennedy. View the logs.


9 months ago

#3 @davidakennedy
9 months ago

  • Milestone changed from Awaiting Review to 4.7

#4 @laurelfulford
9 months ago

  • Keywords has-patch added

Agree that checking for the existence of the header media before outputting is a nice, clean way to fix this. 38927.patch applies this suggestion.

#5 @davidakennedy
9 months ago

  • Keywords commit added
  • Owner set to davidakennedy
  • Status changed from new to assigned

Thanks for the report @LittleBigThing! Nice find!

Tested the patch from @laurelfulford and it works well. This should be good to go.

This ticket was mentioned in Slack in #core by davidakennedy. View the logs.


9 months ago

This ticket was mentioned in Slack in #core by helen. View the logs.


9 months ago

#8 @helen
9 months ago

  • Keywords dev-reviewed added

Nice ticket, @LittleBigThing :) I think props are in order.

#9 @LittleBigThing
9 months ago

Thanks! :)

#10 @davidakennedy
9 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 39392:

Twenty Seventeen: Make fixed navigation apply at correct height on front page, without header video or image

Props laurelfulford, littlebigthing.

Fixes #38927.

#11 @helen
9 months ago

In 39394:

Twenty Seventeen: Make fixed navigation apply at correct height on front page, without header video or image

Props laurelfulford, littlebigthing.

Merges [39392] to the 4.7 branch.
Fixes #38927 for 4.7.

Note: See TracTickets for help on using tickets.