Problem/Motivation

The small custom logo is not center aligned in collapsed state.

https://www.drupal.org/project/drupal/issues/3443810#comment-15575307

Steps to reproduce

1. Goto Navigation settings page.
2. Choose Custom Logo from Logo options.
3. Upload a small logo (5x7)
4. Check the horizontal alignment of the logo in collapsed toolbar state.

Proposed resolution

Probably it should be centered in this case?
TBD

Remaining tasks

Update issue summary
Review

User interface changes

Before:

before

After:

after

API changes

N/A

Data model changes

N/A

Release notes snippet

N/A

Issue fork drupal-3444391

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

finnsky created an issue. See original summary.

finnsky’s picture

Issue tags: +Novice
finnsky’s picture

Title: Center small navigation logo on mobile » Center small navigation logo on collapsed state

adwivedi008 made their first commit to this issue’s fork.

adwivedi008’s picture

@finnsky Can confirm the issue only happens when the inspect element is open and the toolbar gets a scrollbar on it

I am attaching a screen record as a reference and looking into this issue.

ahsannazir made their first commit to this issue’s fork.

ahsannazir’s picture

Status: Active » Needs work
ahsannazir’s picture

Status: Needs work » Needs review
finnsky’s picture

Status: Needs review » Needs work

Thank you for your work. I've added comment with suggestions.

sakthi_dev made their first commit to this issue’s fork.

ahsannazir’s picture

@finnsky Thanks for MR feedback. I have used width instead of max-width with the intention to make it look similar to the default logo dimensions. If using max-width, the small image logos will be little left aligned so thought of giving the width of 40px(same as default logo).

immaculatexavier made their first commit to this issue’s fork.

immaculatexavier’s picture

Status: Needs work » Needs review
smustgrave’s picture

Status: Needs review » Needs work

MR appears to have failures now.

FYI @immaculatexavier and @sakthi_dev this issue was tagged for novice for new users. Looking at your posts you have several 100 posts so probably can avoid novice level issues.

ckrina’s picture

Issue tags: +Portland2024
ahsannazir’s picture

Status: Needs work » Needs review
divya.sejekan’s picture

Getting error while applying patch

finnsky’s picture

@divya.sejekan

Could you please add more informative feedback? Which patch, which error? Thank you

jrperry’s picture

We're working on this in the DrupalCon 2024 mentoring contrib room. We will:

  • Test the patch.
  • Update the status if needed.
kanchan bhogade’s picture

StatusFileSize
new109.91 KB

Hi
I've tried to apply MR !7897 on drupal 11.x
but Getting error while appling MR

Adding Error SS for reference

smustgrave’s picture

Issue summary: View changes
Status: Needs review » Needs work
Issue tags: +Needs issue summary update

For good practice lets complete the issue summary.

Got it started.

ahsannazir’s picture

Issue summary: View changes
ahsannazir’s picture

Issue summary: View changes
ahsannazir’s picture

Status: Needs work » Needs review
skaught’s picture

Status: Needs review » Needs work

max-width: var(--admin-toolbar-space-40);. no more px (:

Wwe need to make sure the image is v/h centered in a 40px A tag that wraps the custom image used. remember #3443810: Custom Navigation logo is disconnected from new Layout template is also coming soon, images will be scaled down.

ahsannazir’s picture

Status: Needs work » Needs review
needs-review-queue-bot’s picture

Status: Needs review » Needs work
StatusFileSize
new90 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. 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.

skaught’s picture

ahsannazir’s picture

Status: Needs work » Needs review
vinmayiswamy’s picture

StatusFileSize
new198.05 KB
new164.33 KB

Hi, I’ve tested MR 7897 on Drupal 11.x.
The MR applied cleanly.

Testing steps:
1. Enable the Navigation module.
2. Goto the module’s settings page.
3. Choose a custom logo from the logo options.
4. Upload a small logo.
5. Check the horizontal alignment of the logo in the collapsed toolbar state.

Test Result:
The logo position has been centered after the MR changes.

Attaching screenshots for reference.

Thanks!

skaught’s picture

Status: Needs review » Needs work
StatusFileSize
new7.19 KB
new7.19 KB

logo is not aligned with rest of first items.

skaught’s picture

ahsannazir’s picture

StatusFileSize
new5.96 KB

This only happens when the vertical scroll bar appears. The rest of icons are not center aligned when the scroll appears. Not sure what should be done to solve this case.

ahsannazir’s picture

Status: Needs work » Needs review
finnsky’s picture

I've added one small comment. Could you please take a look?

skaught’s picture

re-started from up to date clean branch

needs-review-queue-bot’s picture

Status: Needs review » Needs work
StatusFileSize
new2.61 KB

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.

ahsannazir’s picture

Status: Needs work » Needs review

SKAUGHT changed the visibility of the branch 3444391-center-small-navigation to hidden.

skaught’s picture

@ahsannazir thanks for the followup. hiding old branch to reduce confusion.

finnsky’s picture

Status: Needs review » Needs work

1. I think small logo should not move on collapse/expand

https://gyazo.com/084fb9ee2af2b6399caa70f122536303

2. one comment in MR about RTL

3. I don't think test.png image should be in repo.

sheetal.pathak’s picture

StatusFileSize
new128.24 KB
new180.22 KB

I have reviewed this patch.
I have applied MR !8893 on drupal 11.x, applies without any issue.

Steps to Reproduce:
1. Enable the Navigation module.
2. Goto the module’s settings page.
3. Choose a custom logo from the logo options.
4. Upload a small logo.
5. Check the horizontal alignment of the logo in the collapsed toolbar state.

The logo position has been centred after the MR changes.

Attaching screenshots for reference.

Status of this issue can me moved ahead

RTBC +

rodrigoaguilera’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: +Barcelona2024

I think the status change was missing

nod_’s picture

Status: Reviewed & tested by the community » Needs work

Not sure how I feel about all this CSS for just that, but with one more css line it works.

skaught’s picture

Status: Needs work » Needs review
StatusFileSize
new901.64 KB

preview with flex-shrink attached, shows both of our toggles between breakpoints.

local demo image was just 71*83 pixel grab (then autosized as expected to 34x40).

sagarmohite0031’s picture

StatusFileSize
new82.14 KB
new107.97 KB

Hi, I’ve tested MR 8893 on Drupal 11.x.
The MR applied cleanly.

Testing steps:
1. Enable the Navigation module.
2. Goto the module’s settings page.
3. Choose a custom logo from the logo options.
4. Upload a small logo.
5. Check the horizontal alignment of the logo in the collapsed toolbar state.

Test Result:
The logo position has been cantered after the MR changes. But there should be some space between logo and slider.

Attaching screenshots for reference.

skaught’s picture

[opps. removed]

nayana_mvr’s picture

Issue summary: View changes
StatusFileSize
new285.65 KB
new285.18 KB
new703.09 KB

Verified the changes in MR!8893 in Drupal version 11.x and can confirm that it fixes the issue. The position remained centre aligned even while scrolling the navigation bar (please refer the screen recording). Attaching before and after screenshots as well. Need RTBC+1

Before:

before

After:

after

Updated IS with latest screenshots.

nayana_mvr’s picture

Issue summary: View changes
skaught’s picture

Status: Needs review » Reviewed & tested by the community

As per #50 and #48. I'll move to RTBC logo is aligned with the rest of items below, matching overall design mocks.

#48 notes. " But there should be some space between logo and slider." I would agree the width of the entire item might ought to be wider but that scope that might pushing this ticket IMO. Also, as we know various browsers use different widths.

needs-review-queue-bot’s picture

Status: Reviewed & tested by the community » Needs work
StatusFileSize
new90 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. 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.

jvbrian’s picture

Status: Needs work » Needs review
StatusFileSize
new46.36 KB
new7.82 KB

I reviewed the changes and noticed that the issue is resolved in the Drupal version. The position stayed centered; then I moved the navigation bar, and it maintained its position without any issues.

jvbrian’s picture

Status: Needs review » Reviewed & tested by the community
nod_’s picture

Status: Reviewed & tested by the community » Needs work

There is a merge issue to resolve

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.