fix: correct issue with Mobile CTA gap

[laurelfulford]

All Submissions:

• Have you followed the Newspack Contributing guideline?
• Does your code follow the WordPress' coding standards and VIP Go coding standards?
• Have you checked to ensure there aren't other open Pull Requests for the same update/change?

Changes proposed in this Pull Request:

Pairing the short header, simplified subpage header, and mobile CTA together can cause weird gaps in the header. This is because the 'default' and 'short' header switch to the mobile version of the header at different breakpoints, and this was not taken into consideration with how the simplified subpage header works.

This PR (hopefully!) addresses that by making the simplified subpage header more consistent with the header used on the homepage.

Closes #1466.

How to test the changes in this Pull Request:

1. Set up your site header with the following settings:

• Under Customizer > Header Settings > Appearance, select 'Short Header' and unselect 'Center Logo'
• Under Customizer > Header Settings > Subpage Header, turn on the simplified subpage header
• Under Customizer > Header Settings > Mobile CTA, enable the Mobile Call to Action.

2. View the front end of the site, and navigate to a subpage.
3. Shrink your browser window; at a certain point (less than 960px wide, but more than 782px wide), the Mobile CTA should appear, even though desktop-like elements, like the search, are still visible. The CTA should be weirdly floating in the middle:

4. Apply the PR and run npm run build.
5. Refresh the subpage; if you haven't changed the browser window size, the gap should be gone, and the header should be more of the 'mobile's style header:

6. Scale up and down the browser window on this page, and make sure no new weird issues are introduced.
7. Navigate to the homepage, and test the different screen sizes; make sure no new weird issues are introduced:

8. Navigate to Customizer > Header Settings > Mobile CTA, and turn on the mobile CTA on the simplified subpage header.
9. Return to one of your subpages, and make sure no new weird issues are introduced on different screen sizes -- the mobile CTA should always be visible, and not jump around.
10. Navigate to Customizer > Header Settings > Appearance, and check 'Center Logo'; check the homepage and subpage on different screen sized and confirm there aren't any issues.
11. Navigate to Customizer > Header Settings > Appearance, and uncheck 'Short Logo'; check the homepage and subpage on different screen sized and confirm there aren't any issues.
12. Navigate to Customizer > Header Settings > Appearance, and uncheck 'Center Logo'; check the homepage and subpage on different screen sized and confirm there aren't any issues.

Other information:

• Have you added an explanation of what your changes do and why you'd like us to include them?
• Have you written new tests for your changes, as applicable?
• Have you successfully ran tests with your changes locally?

[dkoo]

Ran into a weird style with Center Logo on (step 10) on non-simplified headers. With a site description, at the breakpoint between 780px and 980px, the site description ends up floating weirdly below and to the right of the logo:

I would expect the behavior at this breakpoint to look more like it does without Center Logo on:

Separately, I also notice that on the homepage at the smallest breakpoint, my default/square logo seems to have a bit of extra side padding that doesn't appear at other breakpoints (this seems to happen any combination of header/logo settings). This also happens with the subpage header but only on AMP pages:

I would expect this to be flush-left like it is at larger breakpoints.

Lastly, at the smallest breakpoint the admin bar overlaps the site header. This only seems to happen on non-AMP pages, though, so it's probably an edge case.

[yogeshbeniwal]

Separately, I also notice that on the homepage at the smallest breakpoint, my default/square logo seems to have a bit of extra side padding that doesn't appear at other breakpoints (this seems to happen any combination of header/logo settings).

@laurelfulford This is same as we were discussing here

[laurelfulford]

Ran into a weird style with Center Logo on (step 10) on non-simplified headers. With a site description, at the breakpoint between 780px and 980px, the site description ends up floating weirdly below and to the right of the logo:

Separately, I also notice that on the homepage at the smallest breakpoint, my default/square logo seems to have a bit of extra side padding that doesn't appear at other breakpoints (this seems to happen any combination of header/logo settings). This also happens with the subpage header but only on AMP pages:

Thanks for both of these, @dkoo! They don't actually seem to be relate to these changes, but just kind of weird behaviour that's built up with the header over time as features were added :) Best I can tell, they are:

• Using max-width on a smaller image with AMP enabled - this cause the image to kind of 'float' in the max-width space, adding the extra space on the left and right.
• Spacing around the tagline that doesn't work at all breakpoints - this is causing that left space on the tagline when its sitting under the logo.
• The 'branding' section of the header being too narrow when it switches from being centred on desktop to left-aligned for mobile -- this is causing the tagline to wrap when there's still space for it to sit next to the logo. This is likely because the header switches from 'desktop' layout to 'mobile' layout at a higher breakpoint when you're using the short header, because there's a lot less space for everything than when you're using the default-height header. The centred logo styles seem to stay mostly the same, so the branding only takes up 1/3 of the header, even though it could take up a lot more with the other styles.

I'm working on fixes, but depending on how convoluted they are it may make sense to break them into separate PRs so this doesn't get to be too much to test; I'll follow up once I have that figured that out!

Lastly, at the smallest breakpoint the admin bar overlaps the site header. This only seems to happen on non-AMP pages, though, so it's probably an edge case.

This isn't actually related to this PR - it happens when AMP is half-on for lack of a better way to put it (like if AMP is technically disabled on a page, but you have Campaigns enabled). I did create a theme issue for it here, though it doesn't actually have anything to do with the theme --- it's weird visual issues that happen when you have only some of the styles from AMP, but not everything (if that makes sense!).

@laurelfulford This is same as we were discussing here

@yogeshbeniwal That sounded like it was an issue with extra vertical space around the logo, rather than extra horizontal space, so I don't think it's the same thing (unless I've misunderstood your comment there!).

Can you please create a new GitHub issue for that, including the Customizer settings you're using and some screenshots of the issue happening? That could help us recreate the issue so we can dig into it further (or rule out if it is indeed the same thing) -- thank you!

[dkoo]

I'm working on fixes, but depending on how convoluted they are it may make sense to break them into separate PRs so this doesn't get to be too much to test; I'll follow up once I have that figured that out!

That makes sense—I'm okay with moving those two bugs to their own issues and then re-reviewing this PR without testing them, just let me know!

[laurelfulford]

Thanks @dkoo!

I'm now looking at fixing some other issues with the sticky header and logo sizing that I think at least overlap one issue you found here (the smaller logos kind of floating in the middle of the space), so I'm going to break these out into smaller issues so I don't mess anything up 😐

• Weird tagline wrapping: Extra space next to centred tagline when floated #1619
• Floaty logo: Square-shaped logos appear to be 'floating' in some cases with AMP enabled #1618

Thanks for finding these! I'm going to set this back to needing review again, but please let me know if you have any questions or run into further issues!

[dkoo]

Thanks for the new issues—since those encompass all the requested changes, I think we can move ahead with this PR.

[laurelfulford]

Thanks @dkoo!

[matticbot]

🎉 This PR is included in version 1.55.2-alpha.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

[matticbot]

🎉 This PR is included in version 1.55.2 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀