[WIP] Closes #1406 - Sidebar nav changes #1835

akslay · 2022-08-10T22:50:28Z

Description

In an effort to improve content hierarchy (page title and secondary sidebar content), this PR moves the sidebar nav to the sidebar_second region, hides the sidebar nav on mobile (in favor of the off-canvas experience), eliminates the sidebar nav's h2 margin-top, and consolidates sidebar mobile breakpoints to match the mobile nav breakpoint.

Review site: https://7b5e2985-cedf-4a13-ab91-3a35916c835d--pr-1835.probo.build/

Related issues

Closes #1406

Related to #1834

How to test

Types of changes

Arizona Quickstart (install profile, custom modules, custom theme)

Drupal core

Patch release changes
- Security update
- Patch level release (non-security bug-fix release)
- Patch removal that's no longer necessary
Minor release changes
- Major or minor level update
Other or unknown

Drupal contrib projects

Patch release changes
- Security update
- Patch or minor level update
- Add new module
- Patch removal that's no longer necessary
Minor release changes
- Major level update
Other or unknown

Checklist

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.

joeparsons

Looks good to me.

joshuasosa

The sidebar navigation supports unlimited levels. It doesn't look like the offcanvas nav is taking that into account.

Example on probo: https://7b5e2985-cedf-4a13-ab91-3a35916c835d--pr-1835.probo.build/pages/text

Example of more nav levels

Nav levels missing from mobile

Additionally, the lack of top margin for the navigation looks bad in my opinion.

But nice to see the navigation move to second sidebar. Since most of our users read left to right, my department considered the main page text to be most important text to read first and has had sidebar navigation on the right for many years.

akslay · 2022-08-12T16:40:20Z

I believe the increase of levels in the off-canvas nav is happening in a separate PR that will be related to the design mockups that are coming from the designers. Is that correct @danahertzberg ?

danahertzberg · 2022-08-12T16:59:33Z

I believe the increase of levels in the off-canvas nav is happening in a separate PR that will be related to the design mockups that are coming from the designers. Is that correct @danahertzberg ?

Yes, this is correct

danahertzberg · 2022-08-12T17:34:47Z

I'm personally conflicted between adding or removing top margin.

Please wait to merge until we run by leadership.

akslay · 2022-08-12T17:47:41Z

It sounds like more workshop time needs to happen with this topic; there's some conflicting opinions on sidebar nav placement we should hash out. Going to postpone this PR until we can discuss it in another workshop.

I also think it would be a good idea to remove the "hide on mobile" for the sidebar nav from this PR and instead add it to the PR that addresses adding more tiers to the off-canvas mobile nav. This PR should focus on moving (if still applicable after 2nd round group discussion) the block only.

Carris-Toupin · 2022-08-12T18:59:46Z

We need to compile a list of sites that might be negatively impacted by this 'automatic' change in sidebar position.

joshuasosa · 2022-08-12T23:52:45Z

This PR should focus on moving (if still applicable after 2nd round group discussion) the block only.

Sounds good!

Perhaps changes like moving sidbar nav to the right and/or removing top margin could be done for 'new' sites while 'existing' sites could keep things relatively as-is to reduce the potential for negative impact. For margin, that could mean using an update to put in an mt-4 or mt-5 for existing sites while keeping the default empty for new sites.

trackleft · 2022-08-17T23:15:16Z

Blocks or should be merged together with #1834 if merged.

akslay · 2022-08-17T23:16:40Z

Bringing over Dana's comments from the issue to the PR:

To reduce impact on existing sites and to marry nicely with the page title region changes, this ticket will only require removing the sidebar nav's block title top margin. By changing mt-md-5 to mt-0.

az_quickstart/themes/custom/az_barrio/templates/blocks/block--az-barrio-sidebar-menu.html.twig

Line 49 in 6c816a3

<h2{{ title_attributes.setAttribute('id', heading_id).addClass('h4').addClass('mt-md-5') }}>{{ configuration.label }}</h2>

Also keep Ashley's updates to match region breakpoints to mobile nav breakpoints.

PR requirements have changed

akslay · 2022-08-17T23:29:55Z

PR is updated with latest requirement changes and is ready for re-review and/or approvals.

joshuasosa · 2022-08-18T00:40:45Z

themes/custom/az_barrio/templates/blocks/block--az-barrio-sidebar-menu.html.twig

@@ -46,7 +46,7 @@
    {% set title_attributes = title_attributes.addClass('sr-only') %}
  {% endif %}
  {{ title_prefix }}
-  <h2{{ title_attributes.setAttribute('id', heading_id).addClass('h4').addClass('mt-md-5') }}>{{ configuration.label }}</h2>
+  <h2{{ title_attributes.setAttribute('id', heading_id).addClass('h4').addClass('mt-0') }}>{{ configuration.label }}</h2>


This change immediately affects existing sites. Please change to only affect new sites. If the idea is that such an update would come in another issue/PR, then this PR will be blocked until that is implemented.

I'll supply a patch so we can test this in combination with the #1834 so we can test this easily on existing sites and figure out a way to do the least amount of harm.

I can't think of a way to only affect existing sites, without rewriting this template a bit first.
Steps would be:

Move the added classes into a theme setting. mt-0

Add an update that adds the configuration setting, and sets it to what it was before this change. mt-md-5

Not 100% sure that this will work ☝️ , so will need to test it.

The theory is that config distro will skip a changed value if using the merge strategy.

Additionally, we can update this setting in bulk via drush on any sites we deem safe, after this.

Of course this will most likely be a problem for any sites that have a sub theme with an overridden template and those added classes changed and are using {{title_attributes}}. Unless we don't add classes to title attributes in the .theme file, and instead add the classes to title_attributes within this template.

trackleft · 2022-08-18T18:37:29Z

Patch for testing on existing sites. Add this to you're site specific composer.json.

"extra": {
    "patches": {
       "az-digital/az_quickstart": {
          "test https://github.com/az-digital/az_quickstart/pull/1834" : "https://patch-diff.githubusercontent.com/raw/az-digital/az_quickstart/pull/1834.diff",
          "test https://github.com/az-digital/az_quickstart/pull/1835" : "https://patch-diff.githubusercontent.com/raw/az-digital/az_quickstart/pull/1835.diff"
    }
  }
}

Run composer update

Run drush cr

Do not commit this to your main/master branch. Or add this to your live site, this should be throwaway code.

trackleft · 2022-08-18T19:56:59Z

Something like

/**
 * Add sidebar block classes to config, and setting it to a value that won't break existing sites.
 */
function az_quickstart_update_920104() {
  $config_factory = \Drupal::configFactory();
  $config = $config_factory->getEditable('az_barrio.settings');
  $config->set('sidebar_menu_classes', ['h4', 'mt-md-5']);
  $config->save(TRUE);
}

danahertzberg · 2022-08-19T19:02:49Z

@Carris-Toupin

To get comparison screenshots of existing sites in a multidev:

Move page title block to content top region
Change margin top in inspector from mt-md-5 to mt-0
a. On h2 element of sidebar nav title
View page comparisons/capture screenshots not logged in
Put screenshots in Google Slides and share link on this PR

joshuasosa · 2022-08-19T23:42:54Z

Couple comparisons per Dana's suggestion above.

The nav menu without top margin puts it right against the text on media, which doesn't look good.

Nav with top margin

Nav without top margin

danahertzberg · 2022-08-22T21:22:52Z

In this specific example, the page content can be modified to get the desired look. Add bottom spacing to the Text on Media paragraph, and add the mt-0 class to the H2 element.

It will be important to see the distribution of how this change will look on various sites to determine impact.

Carris-Toupin · 2022-08-31T17:02:02Z

Here are some examples. I need to do more. https://docs.google.com/presentation/d/1ghzsQatd9bUAusnSIzzccpPVisp-DTy4wJwCA8ZM9dE/edit#slide=id.p

danahertzberg · 2022-09-07T17:59:35Z

Potential path forward:

Change template from mt-md-5 to mt-0 (keep as it currently is)
Put mt-md-5 class into block CSS class(es) field with db update

This requires many configuration changes that are fragile.

Will continue discussions int he workshop. Need more research on impact.

trackleft · 2023-01-06T18:00:39Z

Add default theme setting for block title margin class
Set the default to existing setting for existing sites
Set the default to new setting for new sites
Inform per block setting from theme setting and allow override.

danahertzberg · 2024-05-31T18:31:41Z

This may need to be revisited with the upcoming navigation changes led by central MarCom.

Sidebar nav changes

ef67977

akslay added enhancement New feature or request visual change Introduces a visual change labels Aug 10, 2022

akslay self-assigned this Aug 10, 2022

akslay requested a review from a team as a code owner August 10, 2022 22:50

joeparsons reviewed Aug 10, 2022

View reviewed changes

joeparsons previously approved these changes Aug 10, 2022

View reviewed changes

akslay mentioned this pull request Aug 10, 2022

Closes #1822 - Move page title block to content_top region #1834

Closed

27 tasks

joshuasosa previously requested changes Aug 11, 2022

View reviewed changes

trackleft added the blocker label Aug 17, 2022

Workshop updates

c6a10be

akslay dismissed joeparsons’s stale review via c6a10be August 17, 2022 23:18

Merge branch 'main' into issue/1406

19313d8

Oops

f9478ca

joshuasosa requested changes Aug 18, 2022

View reviewed changes

Merge branch 'main' into issue/1406

dbbe44b

joeparsons added the 2.7.x only label Feb 22, 2023

akslay assigned danahertzberg and unassigned akslay May 31, 2023

joeparsons added the needs discussion Further discussion required to determine requirements label Aug 4, 2023

danahertzberg marked this pull request as draft September 22, 2023 17:04

danahertzberg changed the title ~~Closes #1406 - Sidebar nav changes~~ [WIP] Closes #1406 - Sidebar nav changes Sep 22, 2023

danahertzberg closed this May 31, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Closes #1406 - Sidebar nav changes #1835

[WIP] Closes #1406 - Sidebar nav changes #1835

akslay commented Aug 10, 2022 •

edited

Loading

joeparsons left a comment

joshuasosa left a comment •

edited

Loading

akslay commented Aug 12, 2022

danahertzberg commented Aug 12, 2022

danahertzberg commented Aug 12, 2022

akslay commented Aug 12, 2022 •

edited

Loading

Carris-Toupin commented Aug 12, 2022

joshuasosa commented Aug 12, 2022

trackleft commented Aug 17, 2022

akslay commented Aug 17, 2022

akslay commented Aug 17, 2022

joshuasosa Aug 18, 2022 •

edited

Loading

trackleft Aug 18, 2022 •

edited

Loading

trackleft Aug 18, 2022 •

edited

Loading

trackleft Aug 18, 2022 •

edited

Loading

trackleft commented Aug 18, 2022 •

edited

Loading

trackleft commented Aug 18, 2022 •

edited

Loading

danahertzberg commented Aug 19, 2022

joshuasosa commented Aug 19, 2022

danahertzberg commented Aug 22, 2022

Carris-Toupin commented Aug 31, 2022

danahertzberg commented Sep 7, 2022 •

edited

Loading

trackleft commented Jan 6, 2023

danahertzberg commented May 31, 2024

[WIP] Closes #1406 - Sidebar nav changes #1835

[WIP] Closes #1406 - Sidebar nav changes #1835

Conversation

akslay commented Aug 10, 2022 • edited Loading

Description

Related issues

How to test

Types of changes

Arizona Quickstart (install profile, custom modules, custom theme)

Drupal core

Drupal contrib projects

Checklist

joeparsons left a comment

Choose a reason for hiding this comment

joshuasosa left a comment • edited Loading

Choose a reason for hiding this comment

Example of more nav levels

Nav levels missing from mobile

akslay commented Aug 12, 2022

danahertzberg commented Aug 12, 2022

danahertzberg commented Aug 12, 2022

akslay commented Aug 12, 2022 • edited Loading

Carris-Toupin commented Aug 12, 2022

joshuasosa commented Aug 12, 2022

trackleft commented Aug 17, 2022

akslay commented Aug 17, 2022

Bringing over Dana's comments from the issue to the PR:

akslay commented Aug 17, 2022

joshuasosa Aug 18, 2022 • edited Loading

Choose a reason for hiding this comment

trackleft Aug 18, 2022 • edited Loading

Choose a reason for hiding this comment

trackleft Aug 18, 2022 • edited Loading

Choose a reason for hiding this comment

trackleft Aug 18, 2022 • edited Loading

Choose a reason for hiding this comment

trackleft commented Aug 18, 2022 • edited Loading

trackleft commented Aug 18, 2022 • edited Loading

danahertzberg commented Aug 19, 2022

joshuasosa commented Aug 19, 2022

Nav with top margin

Nav without top margin

danahertzberg commented Aug 22, 2022

Carris-Toupin commented Aug 31, 2022

danahertzberg commented Sep 7, 2022 • edited Loading

trackleft commented Jan 6, 2023

danahertzberg commented May 31, 2024

akslay commented Aug 10, 2022 •

edited

Loading

joshuasosa left a comment •

edited

Loading

akslay commented Aug 12, 2022 •

edited

Loading

joshuasosa Aug 18, 2022 •

edited

Loading

trackleft Aug 18, 2022 •

edited

Loading

trackleft Aug 18, 2022 •

edited

Loading

trackleft Aug 18, 2022 •

edited

Loading

trackleft commented Aug 18, 2022 •

edited

Loading

trackleft commented Aug 18, 2022 •

edited

Loading

danahertzberg commented Sep 7, 2022 •

edited

Loading