Blockbase: Add social navigation to blockbase themes #4482

scruffian · 2021-08-27T12:16:43Z

Changes proposed in this Pull Request:

This adds social navigation to all Blockbase themes:

Seedlet blocks

Mayland Blocks

Geologist

Quadrat

Blockbase:

Skatepark

jffng · 2021-09-01T19:58:19Z

I rebased this now that #4468 is merged. Seedlet and Mayland look okay, but I have two questions about Blockbase and Quadrat:

How should we handle alignment?

One option is adding a margin-left: auto; to the primary nav in CSS.

How should we handle the header when the responsive menu option is enabled?

scruffian · 2021-09-02T08:13:32Z

@kjellr, do you have any thoughts on this?

kjellr · 2021-09-02T16:34:21Z

How should we handle alignment?
One option is adding a margin-left: auto; to the primary nav in CSS.

For desktop, I think we should do that. In general, this should look like the Quadrat screenshot above, but with more margin in between the nav + the social links:

The right margin should be equal to the gap between nav items.

How should we handle the header when the responsive menu option is enabled?

This is another example of where we would ideally put these in the expanded hamburger menu. Part of me wonders if we should hack some temporary jQuery in there to move it there on small screens. 😅 Let me think about this on a little and get back to you later today/tomorrow.

kjellr · 2021-09-03T18:36:56Z

seedlet-blocks/block-template-parts/header.html

@@ -11,6 +11,8 @@
 <!-- wp:navigation {"itemsJustification":"center","isResponsive":true,"__unstableLocation":"primary"} -->
 <!-- /wp:navigation -->

+<!-- wp:navigation {"orientation":"horizontal","itemsJustification":"center","isResponsive":true,"__unstableLocation":"social"} --><!-- /wp:navigation -->


For Seedlet, let's use a default icon color of black (the foreground color) to match the social menu color in the classic version of the theme.

kjellr · 2021-09-03T18:57:51Z

@melchoyce @beafialho what do you think of these options for social links placement on small screens in Blockbase and Quadrat?

None of them seem ideal to me, so I've also offered a plea for a better solution in the issue here.

beafialho · 2021-09-06T13:43:44Z

I think the best and less invasive option is definitely the top middle one, it makes most sense for the social icons to be beneath the hamburger. The lines add way too much noise to the top area (in this case, where these themes don't use them anywhere else).

I agree that the solution you propose in the Navigation block issue should be possible as a default.

kjellr · 2021-09-07T12:09:59Z

Ok, looping back to this. It is possible to put social links within a navigation block, so we should just aim to do that instead of having two separate navigation menus.

scruffian · 2021-10-01T11:48:49Z

I updated the approach here - now the social navigation gets appended to the primary navigation block. This means that it gets wrapped by the hamburger menu when in smaller screens.

We probably need some CSS to fix the alignments of the social menu, but I'm curious to get feedback on the approach before we do that.

pbking · 2021-10-01T12:39:21Z

I really like the approach.

I kind of would prefer to have a secondary attribute to key on for appending the social menu to the block.
(i.e. <-- wp:navigation {__unstableLocation:"primary",__unstableAppendSocial=true} --> or the ability to set __unstableLocation multiple times, or to an array or something)

But presently I think maybe that's something the feature potentially could grow into rather than start at.

It feels like the cleanest way to achieve the design target.

Only beef I have with the code is a preference to move the menu rendering stuff to another file.

scruffian · 2021-10-01T14:29:12Z

Only beef I have with the code is a preference to move the menu rendering stuff to another file.

Good idea, done!

MaggieCabrera · 2021-10-04T08:26:16Z

I'm getting this error when I apply this branch and go to the customizer and add a social menu to the "Social" location:

( ! ) Warning: DOMDocument::loadHTML(): Tag nav invalid in Entity, line: 1 in /Users/maggie/A8c/repos/a8c-projects/themes/blockbase/inc/social-navigation.php on line 48

MaggieCabrera · 2021-10-04T08:31:11Z

blockbase/inc/social-navigation.php

+
+// We should only change the render of the navigtion block
+// to social links in the following conditions.
+function blockbase_condition_to_render_social_menu( $block ) {


A theme may want to define a menu that follows these conditions to show up in the footer too for example, or a pre-footer template part. In some cases we may want to have the social navigation but maybe not always. The perfect example of this is actually Skatepark, with this PR in place, this is what I see when I insert the pre footer block pattern:

scruffian · 2021-10-05T12:10:31Z

I'm getting this error when I apply this branch and go to the customizer and add a social menu to the "Social" location:

Fixed in 21c1ed4

scruffian · 2021-10-05T12:13:19Z

I kind of would prefer to have a secondary attribute to key on for appending the social menu to the block.
(i.e. <-- wp:navigation {__unstableLocation:"primary",__unstableAppendSocial=true} --> or the ability to set __unstableLocation multiple times, or to an array or something)

This turned out to be a very good idea, as it means we can control which menus the links get appended to, as I have done in 3a9f0f5.

If y'all like this approach we can add it to the header nav in all blockbase themes.

MaggieCabrera · 2021-10-05T16:06:02Z

Now it's fataling for me:

scruffian · 2021-10-06T13:04:19Z

I fixed the fatal, so you should be able to test now.

This revealed one shortcoming of this approach - if the primary navigation location is empty then no navigation is output, even if something is set in social...

MaggieCabrera · 2021-10-06T13:09:17Z

I fixed the fatal, so you should be able to test now.

This revealed one shortcoming of this approach - if the primary navigation location is empty then no navigation is output, even if something is set in social...

Yes! no more errors for me. Now that we have __unstableSocialLinks, why do we need to check for the other attribute? I think having __unstableSocialLinks is specific enough for us to know that we want to render the social links

RavanH · 2021-10-06T22:51:21Z

Hi all, I've been testing the current social menu implementation in trunk with a local child theme and it seems to work well. Only concern I have is that (unlike item justification) it's not possible to change/unset icon color and icon size.

Is this feature considered already, and if so, is anybody working on that? If not, I could work on a proposal...

scruffian · 2021-10-07T07:11:55Z

Does it work to set icon color and size in the theme.json?

RavanH · 2021-10-08T13:02:27Z

@scruffian, I tried that but do not think it is possible with this block... IS there any documentation on Social Icons specifically?

scruffian · 2021-10-08T20:20:11Z

I think we'll need something like this for that to work.

scruffian · 2021-10-08T20:20:48Z

For now I think we'll have have to make this a setting for the theme...

pbking · 2021-10-11T16:32:28Z

Now that we have __unstableSocialLinks, why do we need to check for the other attribute? I think having __unstableSocialLinks is specific enough for us to know that we want to render the social links

Pushed a small change with regard to @MaggieCabrera 's comment. No longer requires the parent container to be parent menu; __unstableSocialLinks on a navigation block is the only requirement. Holds true for having content; when there is no content the social links are provided (rather than appended) eliminating the need for the empty() check (and allowing the 'social links' menu target to be exclusively targeted as such:

<!-- wp:navigation {"__unstableSocialLinks":"social"} /-->

I think the existing color management (leveraging primary color for social links) is acceptable and appropriate for the time being.

Otherwise (IMO) this change is good-to-go once the appropriate changes have been made to all of the child themes to both add (and style) the social links.

…or even have content)

scruffian · 2021-10-12T13:18:30Z

I have now added social nav to all Blockbase themes, and updated the CSS to accommodate the new navigation. I have updated the screenshots in the PR description

This should also fix #4590

…ay that the navigation block works

pbking

Boom!
All the themes look great. Seems like a great ponyfill to get things into place.
Diggit
Shippit

scruffian added the [Theme] Blockbase label Aug 27, 2021

scruffian requested a review from a team August 27, 2021 12:16

scruffian self-assigned this Aug 27, 2021

jffng force-pushed the blockbase/add-social-nav branch from 7f0e7f6 to b97a262 Compare September 1, 2021 19:31

kjellr reviewed Sep 3, 2021

View reviewed changes

MaggieCabrera mentioned this pull request Sep 6, 2021

Geologist: enhancements and code quality #4531

Closed

MaggieCabrera mentioned this pull request Sep 8, 2021

Mayland Blocks, Blockbase: fix header vertical spacing #4543

Merged

scruffian mentioned this pull request Sep 10, 2021

Blockbase: A different approach to navigation #4575

Closed

scruffian force-pushed the blockbase/add-social-nav branch from b97a262 to a0c7bbe Compare October 1, 2021 09:13

MaggieCabrera reviewed Oct 4, 2021

View reviewed changes

scruffian force-pushed the blockbase/add-social-nav branch from 8dfa0a4 to d5afbd0 Compare October 5, 2021 11:03

scruffian force-pushed the blockbase/add-social-nav branch from 3e5cc86 to 03f6a10 Compare October 6, 2021 13:03

scruffian and others added 8 commits October 12, 2021 11:34

Blockbase: Add social navigation to blockbase themes

0755cc0

Add social navigation links to primary navigation

fda3f37

move to a different file

1ebd2ea

add missing file

6cf2f26

suppress warnings for HTML5 elements

8b8fbdf

only append social links to a navigation block if the block opts in

382025d

Fix fatal

854f4b3

Removed requirement to have social parent container be primary menu (…

4f6db5b

…or even have content)

scruffian force-pushed the blockbase/add-social-nav branch from f68b2f6 to 4f6db5b Compare October 12, 2021 10:36

Fix layout for social nav

30e1c09

scruffian requested a review from a team October 12, 2021 13:15

Update the structure of the social links when appended to match the w…

676738b

…ay that the navigation block works

scruffian mentioned this pull request Oct 12, 2021

Skatepark: Social Links navigation is not properly aligned on desktop #4590

Closed

pbking approved these changes Oct 12, 2021

View reviewed changes

scruffian merged commit c23cac3 into trunk Oct 12, 2021

scruffian deleted the blockbase/add-social-nav branch October 12, 2021 13:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Blockbase: Add social navigation to blockbase themes #4482

Blockbase: Add social navigation to blockbase themes #4482

scruffian commented Aug 27, 2021 •

edited

Loading

jffng commented Sep 1, 2021

scruffian commented Sep 2, 2021

kjellr commented Sep 2, 2021

kjellr Sep 3, 2021

kjellr commented Sep 3, 2021 •

edited

Loading

beafialho commented Sep 6, 2021

kjellr commented Sep 7, 2021

scruffian commented Oct 1, 2021

pbking commented Oct 1, 2021 •

edited

Loading

scruffian commented Oct 1, 2021

MaggieCabrera commented Oct 4, 2021

MaggieCabrera Oct 4, 2021

scruffian commented Oct 5, 2021

scruffian commented Oct 5, 2021

MaggieCabrera commented Oct 5, 2021

scruffian commented Oct 6, 2021

MaggieCabrera commented Oct 6, 2021

RavanH commented Oct 6, 2021

scruffian commented Oct 7, 2021

RavanH commented Oct 8, 2021

scruffian commented Oct 8, 2021

scruffian commented Oct 8, 2021

pbking commented Oct 11, 2021

scruffian commented Oct 12, 2021

pbking left a comment

Blockbase: Add social navigation to blockbase themes #4482

Blockbase: Add social navigation to blockbase themes #4482

Conversation

scruffian commented Aug 27, 2021 • edited Loading

Changes proposed in this Pull Request:

jffng commented Sep 1, 2021

scruffian commented Sep 2, 2021

kjellr commented Sep 2, 2021

kjellr Sep 3, 2021

Choose a reason for hiding this comment

kjellr commented Sep 3, 2021 • edited Loading

beafialho commented Sep 6, 2021

kjellr commented Sep 7, 2021

scruffian commented Oct 1, 2021

pbking commented Oct 1, 2021 • edited Loading

scruffian commented Oct 1, 2021

MaggieCabrera commented Oct 4, 2021

MaggieCabrera Oct 4, 2021

Choose a reason for hiding this comment

scruffian commented Oct 5, 2021

scruffian commented Oct 5, 2021

MaggieCabrera commented Oct 5, 2021

scruffian commented Oct 6, 2021

MaggieCabrera commented Oct 6, 2021

RavanH commented Oct 6, 2021

scruffian commented Oct 7, 2021

RavanH commented Oct 8, 2021

scruffian commented Oct 8, 2021

scruffian commented Oct 8, 2021

pbking commented Oct 11, 2021

scruffian commented Oct 12, 2021

pbking left a comment

Choose a reason for hiding this comment

scruffian commented Aug 27, 2021 •

edited

Loading

kjellr commented Sep 3, 2021 •

edited

Loading

pbking commented Oct 1, 2021 •

edited

Loading