Needs work
Project:
Drupal core
Version:
main
Component:
navigation.module
Priority:
Normal
Category:
Feature request
Assigned:
Unassigned
Reporter:
Created:
30 Apr 2024 at 12:45 UTC
Updated:
26 Oct 2024 at 12:22 UTC
Jump to comment: Most recent, Most recent file


Comments
Comment #2
finnsky commentedComment #3
finnsky commentedComment #5
adwivedi008 commented@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.
Comment #8
ahsannazir commentedComment #9
ahsannazir commentedComment #10
finnsky commentedThank you for your work. I've added comment with suggestions.
Comment #12
ahsannazir commented@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).
Comment #14
immaculatexavier commentedComment #15
smustgrave commentedMR 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.
Comment #16
ckrinaComment #17
ahsannazir commentedComment #18
divya.sejekan commentedGetting error while applying patch
Comment #19
finnsky commented@divya.sejekan
Could you please add more informative feedback? Which patch, which error? Thank you
Comment #20
jrperry commentedWe're working on this in the DrupalCon 2024 mentoring contrib room. We will:
Comment #21
kanchan bhogade commentedHi
I've tried to apply MR !7897 on drupal 11.x
but Getting error while appling MR
Adding Error SS for reference
Comment #22
smustgrave commentedFor good practice lets complete the issue summary.
Got it started.
Comment #23
ahsannazir commentedComment #24
ahsannazir commentedComment #25
ahsannazir commentedComment #26
skaughtmax-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.
Comment #27
ahsannazir commentedComment #28
needs-review-queue-bot commentedThe 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.
Comment #29
skaughtAlso now #3443810: Custom Navigation logo is disconnected from new Layout template is in. Branch needs to be updated.
Comment #30
ahsannazir commentedComment #31
vinmayiswamy commentedHi, 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!
Comment #32
skaughtlogo is not aligned with rest of first items.
Comment #33
skaughtComment #34
ahsannazir commentedThis 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.
Comment #35
ahsannazir commentedComment #36
finnsky commentedI've added one small comment. Could you please take a look?
Comment #38
skaughtre-started from up to date clean branch
Comment #39
needs-review-queue-bot commentedThe 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.
Comment #40
ahsannazir commentedComment #42
skaught@ahsannazir thanks for the followup. hiding old branch to reduce confusion.
Comment #43
finnsky commented1. 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.
Comment #44
sheetal.pathak commentedI 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 +
Comment #45
rodrigoaguileraI think the status change was missing
Comment #46
nod_Not sure how I feel about all this CSS for just that, but with one more css line it works.
Comment #47
skaughtpreview 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).
Comment #48
sagarmohite0031 commentedHi, 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.
Comment #49
skaught[opps. removed]
Comment #50
nayana_mvr commentedVerified 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:
After:
Updated IS with latest screenshots.
Comment #51
nayana_mvr commentedComment #52
skaughtAs 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.
Comment #53
needs-review-queue-bot commentedThe 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.
Comment #54
jvbrian commentedI 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.
Comment #55
jvbrian commentedComment #56
nod_There is a merge issue to resolve