Add desktop slideout menu

[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:

This PR starts the process of adding a 'slide out' menu to the header (#766).

It's opened up some questions for me, so I wanted to push up an in-progress PR and get some feedback. CCing @philipjohn (since you're working with the publications asking for this functionality), and @thomasguillot for a second opinion on how to handle it.

How do we work yet another button into the menu?

In working with the code, I think the best spot is to the left of the secondary menu. Unfortunately, at the moment, that secondary menu is not visible if you pick the 'Short Header' option. So if we went with this placement, I think that secondary menu space would need to be visible if you have a secondary menu and/or desktop slide out menu. The header overall would still be shorter -- the primary menu would still be in line with the logo -- but it will make the header taller.

Is there another placement I'm not thinking of?

Should this space allow more than menus?

Right now, the PR adds just one menu to this desktop space, and moves it into the mobile menu when viewed on smaller screens.

I don't want to over-complicate it, but I feel like we'll be asked about adding other simple things to this space -- like text, or other widget-like content. Which makes me wonder:

• Should this area have a widget area?
• Should it only have widget areas, and no menu (since there is a Navigation Menu widget)?
• Can we/should we try to do something hacky with a page/gutenberg editor instead? On one hand: freedom! On the other hand: I feel like this could get very ugly, very quickly.
• What should happen with this content on mobile, knowing it could be very complicated? I'm leaning towards it being hidden (similar to the Highlight Menu), but realize that could be different than expected.

How to test the changes in this Pull Request:

To test what's currently in this PR:

1. Apply the PR and run npm run build.
2. Navigate to Customize > Menus, and add a Menu to the "Desktop Sidebar Menu" (note to self: this should be changed to be less specific).
3. If not already, navigate to Customize > Header Settings and uncheck 'Short Header'.
4. View on front-end; note there's a Menu link in line with the Secondary Menu:

5. Click to open the menu -- it is currently set up to display two levels of a menu, treating the top level links like headers. This could also be updated to act more like the Primary menu on mobile, allowing multiple levels:

6. Scale down the browser window, and open the regular mobile menu. Confirm that your Desktop Sidebar menu is there, too, under the Secondary menu:

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?

[thomasguillot]

Should it only have widget areas, and no menu (since there is a Navigation Menu widget)?

This.

And ideally, the ability to edit the "Menu" label (theme option?)

It could be something like:

• Enable widget area in Header

↓

• Enable widget area in Header
  Button Label: ________

Only visible when a site isn't using the Short Header.

[laurelfulford]

And ideally, the ability to edit the "Menu" label (theme option?)

Oh yeah, that's a great idea! It'll be the first thing someone will want to change 😄 Thanks Thomas!

[laurelfulford]

I've updated this PR in a few ways, detailed below. I'm marking as ready for review, but am still very interested in feedback on the approach: 😬

Updates include:

• Switching the contents of the slide out from a menu to a widget area. As part of this I also made the slideout a bit narrower, and removed the two column styles for increased flexibility.
• Adding a toggle to show/hide the slide-out sidebar in the Header Settings panel. If turned on, you have two more options:
  • Adding a text field to edit the label text.
  • Adding a toggle to also include these widgets in the mobile menu.
• Added links between the Header Settings toggle and the related widgets area, to make it easier to set up (in theory!).

How to test the changes in this Pull Request:

1. If not already, navigate to Customize > Header Settings, and turn off 'Short Header' to start.
2. If not already, set AMP to standard more.
3. Apply the PR and run npm run build.
4. Navigate to Customize > Widgets, and confirm there's now a "Slide-out Sidebar" widget area.
5. Add some widgets to this area -- a variety of types -- and save and publish.
6. Click on the 'Header Settings' link in the panel's description:

7. This should open the Header Settings panel, at the bottom of which should be an option to 'Show Slide-out Sidebar'. Check this option:

8. Confirm that there's now a 'Menu' toggle in the top corner of the header, and two more options appear below the checkbox -- one to change the text label of the toggle, and one to also add the same widgets to the mobile menu:

9. Save and Publish.
10. Click on the Menu toggle; confirm that the menu opens, and that your widgets display, with a border separating each of them:

11. Navigate back to Customize > Header Settings.
12. Try changing the Slide out Sidebar Text to something else.
13. Check 'Add slide-out widgets to mobile menu'.
14. Save and publish.
15. Confirm your toggle is now using the new label:

13. Shrink down the browser window until the mobile menu appears, and open it. Confirm your widgets now display there, and don't have display issues. They should appear under all of your menus:

14. Navigate to Customize > Header Settings, and check 'Center Logo'; save and view on the front end to confirm there are no visual issues (nothing should have changed around the toggle):

15: Navigate to Customize > Header Settings, and check 'Short Header'; save and view on the front end to confirm there are no visual issues. The toggle should appear in line with the logo, next to the social nav links, but with no visible label:

16. Navigate back to Customize > Header Settings, uncheck 'Center Logo'; save and view on the front end to confirm there are no visual issues:

Some questions:

• What do you think about hiding the menu toggle text on the small screen?
• The sidebar toggle will appear only when you turn it on under Header Settings, and it will show even if the widget area is empty. I didn't want to make the conditions around when it showed too complicated, but do you think it's okay that you can turn it off and leave it empty?

[thomasguillot]

What do you think about hiding the menu toggle text on the small screen?

I guess it's fine.

The sidebar toggle will appear only when you turn it on under Header Settings, and it will show even if the widget area is empty. I didn't want to make the conditions around when it showed too complicated, but do you think it's okay that you can turn it off and leave it empty?

It's a bit of an edge case. Why would you turn it on and not use it... but I guess a check if is_active_sidebar would make sense to have here.

[thomasguillot]

It looks great and works as expected.

I have a few recommendations:

• For the buttons, instead of using position relative and top: 5px for the svg, you could simply use display: flex and align-items: center.
• The focus on these buttons looks odd since you have outline-offset: -4px. I'd suggest to change this to 0 or even 2/4px
• The margin between elements is not consistent (e.g.)

[laurelfulford]

Thanks Thomas!

It's a bit of an edge case. Why would you turn it on and not use it... but I guess a check if is_active_sidebar would make sense to have here.

Good point! I've added an is_active_sidebar check as well, so you'll need both the slide-out sidebar turned on, and widgets added to it, for it to show.

For the buttons, instead of using position relative and top: 5px for the svg, you could simply use display: flex and align-items: center.

Ah, that's working a lot nicer!

The focus on these buttons looks odd since you have outline-offset: -4px. I'd suggest to change this to 0 or even 2/4px

Updated to 0 -- I think it looks better, but let me know if you disagree!

The margin between elements is not consistent

Ah dang, my styles were relying on widget titles being present, which wouldn't always be the case. This should be consistent now, whether or not you've added a title.

Just let me know if there's anything else!

[thomasguillot]

Looks good! Thanks @laurelfulford!

[philipjohn]

This is amazing, Laurel. Really well thought through - I think folks are going to be very pleased with this.

[laurelfulford]

Thanks Thomas and Phil! Excited to see this land in the next release.

[matticbot]

🎉 This PR is included in version 1.4.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

[laurelfulford]

Docs have been completed for this feature.