Custom Navigation logo is disconnected from new Layout template

Problem/Motivation

Recent change to Layout Builder has left Custom Logo Option without it's path and image size.

Steps to reproduce

Goto Drupal 11 dev line and enable Navigaiton module. goto 'Nafivation settings' config page and setup with a file.

Proposed resolution

Reconnect variable to twig.

Remaining tasks

📌 Add tests for the new feature to change the logo Active .

User interface changes

N/A.

API changes

N/A.

Data model changes

None.

Release notes snippet

Shouldn't the tests be included with this fix?

In general, yes of course! As this break was so late before the MR and has the standing ticket for related tests, this as a direct repair could be seen to given the work now on dev line.

In general, the tests did shift too with moving to layout builder, I don't have a clear base for where to setup some of the testing. #344355: SbT doesn't respect node access? → .

Need Proper Steps to reproduce and screenshots

Hi, I have verified and tested the drupal logo issue in our 11.x dev branch. It is working fine now.

Testing steps:
- Using Drupal 11 dev line, install standard profile.
- enable Navigation module.
- goto 'Navigation settings' config page (admin/config/user-interface/navigation/settings)
- select 'custom logo'
- attach image to 'choose custom logo'
-save config
- notice: Navigation Logo is Still the 'default drupal logo'

Testing Results:
Drupal logo should be changing now when custom logo is uploaded.

Attached Before and After Screenshots as well.

Hence moving this issue to RTBC.

we probably need to define this behaviour:

i uploaded small 5X7 image in tugboat.

Can be follow up issue i guess.

Follow up:
https://www.drupal.org/project/drupal/issues/3444391 ✨ Center small navigation logo on collapsed state Active

I see the test coverage is in a follow up and this is experimental but are we adding fixes without coverage?

Have been in communication around 🐛 Navigation Test names are disjointed. Active and 🐛 Enhance Navigation admin workflow with Managed Tabs. Needs review . It is clear that we have the scope to start a new test for this, this way.

As I have some other work todo and there is much happening next week, i'll take move to needs work so that it may be picked up meanwhile.

I have just added some outlining notes to this WIP in the test page for insights. It does not actually test yet (:

jrperry → made their first commit to this issue’s fork.

Thanks for working on this issue! Was previously tagged for tests which are still needed so moving to NW for those.

SKAUGHT → changed the visibility of the branch 3443810-custom-nav-logo-disconnect-fix to hidden.

Tests for each of the 3 general logo handling options. A bit of a shortcut to setup for op3 to preset the config itself and verify the current fid will be ready for #3436526 and it's tests.

Thank you for working on this.

Took a look locally and icon logic works well.

Added some minor comments in the MR.

It would require a review from someone with better knowledge of the HTML structure to confirm that everything is OK.

Another point I noticed that could confuse users configuring the settings is the lack of clarification that these settings are for the 'Site Logo' and not a specific logo for navigation.

It would be helpful to add a brief description to each option or a general help text to clarify the purpose of these settings.

Thank you.

#32, indeed there are some open conversations around this.

📌 [meta] Improve the navigation layout page Active maybe the best issue to point you to.

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

test are running green again.

Removed a couple of unnecessary dependencies from test class.

I think it is safe to mark this as RTBC now.

Thank you for your efforts here!

Question about the variables available inside the template and comment line length to fix.

i'll return to RTBC. thanks!

Left some questions on the MR - thanks for working on this!

I'll returning to RTBC again, thanks!

nod_ → closed merge request !7834

nod_ → closed merge request !7987

Committed and pushed f479374ee8 to 11.x and 85a4d771c1 to 11.0.x and 58c748be8e to 10.4.x. Thanks!

Automatically closed - issue fixed for 2 weeks with no activity.