Problem/Motivation

- This particular issue came out from the design QA review process conducted by @jwitkowski79.
- On Tablet & Mobile: The letter spacing for subnav items looks super tight. Can we remove the letter-spacing here?

Steps to reproduce

- Open the slideout mobile menu.

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#17 after-patch.png6.75 KBsulfikar_s
#17 before-patch.png6.7 KBsulfikar_s
#17 failure-case.png6.63 KBsulfikar_s
#16 3180280-15.patch1.23 KBmherchel
#14 rtl.png322.47 KBkomalk
#14 ltr.png396.97 KBkomalk
#14 interdiff_9-14.txt896 byteskomalk
#14 3180280-14.patch994 byteskomalk
#10 before--patch.jpg79.69 KBranjith_kumar_k_u
#10 after--patch.jpg79.37 KBranjith_kumar_k_u
#9 Welcome to drupal9 _ drupal9.mp4354.47 KBkomalk
#9 interdiff_3-9.txt1.25 KBkomalk
#9 3180280-9.patch997 byteskomalk
#5 scrnli_03_11_2020_10-51-29.png424.81 KBkomalk
#5 scrnli_03_11_2020_10-50-01.png75.35 KBkomalk
#5 3180280-3.patch1003 byteskomalk
c58Euuxg.png498.4 KBproeung

Comments

proeung created an issue. See original summary.

komalk’s picture

komalk commented
Assigned: Unassigned » komalk
akshay kashyap’s picture

akshay kashyap commented
Assigned: komalk » akshay kashyap
akshay kashyap’s picture

akshay kashyap commented
Assigned: akshay kashyap » Unassigned
komalk’s picture

komalk commented
Status: Active » Needs review
StatusFileSize
new 3180280-3.patch1003 bytes
new scrnli_03_11_2020_10-50-01.png75.35 KB
new scrnli_03_11_2020_10-51-29.png424.81 KB

Remove the letter-spacing on sub-nav items and attached the screenshot for references.
Review the patch.

akshay kashyap’s picture

akshay kashyap commented
Assigned: Unassigned » akshay kashyap
mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
Assigned: akshay kashyap » Unassigned
Status: Needs review » Needs work

The image within the summary only points to the second level navigation. You updated the letter spacing for the entire mobile navigation.

We need to ensure that we only affect the mobile second level navigation.

komalk’s picture

komalk commented
Assigned: Unassigned » komalk
komalk’s picture

komalk commented
Assigned: komalk » Unassigned
Status: Needs work » Needs review
StatusFileSize
new 3180280-9.patch997 bytes
new interdiff_3-9.txt1.25 KB
new Welcome to drupal9 _ drupal9.mp4354.47 KB

Worked on #7.
Targeting the letter-spacing only for the second level also attached file for the references.

ranjith_kumar_k_u’s picture

ranjith_kumar_k_u commented
StatusFileSize
new after--patch.jpg79.37 KB
new before--patch.jpg79.69 KB

The above patch works fine .
Before patch
before patch
After Patch
after patch
RTBC

ranjith_kumar_k_u’s picture

ranjith_kumar_k_u commented
Status: Needs review » Reviewed & tested by the community
mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/themes/olivero/css/components/navigation/nav-primary.pcss.css
@@ -120,6 +120,9 @@
+  letter-spacing: 0px; ¶

Thanks for the patch! There are a couple issues:

1) There's trailing whitespace..
2) No need for the px unit
3) Instead of overriding this here, I'd rather move the letter-spacing value to the .primary-nav__menu--level-1 selector.

komalk’s picture

komalk commented
Assigned: Unassigned » komalk

Working on.

komalk’s picture

komalk commented
Assigned: komalk » Unassigned
Status: Needs work » Needs review
StatusFileSize
new 3180280-14.patch994 bytes
new interdiff_9-14.txt896 bytes
new ltr.png396.97 KB
new rtl.png322.47 KB

Work on 12.1,12.2.
12.3 - If we move the letter-spacing into .primary-nav__menu--level-1not geeting the expected output.
Attached screenshot for the LTR & RTL.

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
Status: Needs review » Needs work

#14 doesn't move the letter-spacing value to the .primary-nav__menu--level-1 selector as requested

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
Status: Needs work » Needs review
StatusFileSize
new 3180280-15.patch1.23 KB

Updated patch attached!

sulfikar_s’s picture

sulfikar_s commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new failure-case.png6.63 KB
new before-patch.png6.7 KB
new after-patch.png6.75 KB

Hi, the patch in #15 applied cleanly and it works,

Before patch,

before-patch.png

After patch,

after-patch.png

But I've found random failures in the #15 patch's test cases regarding,

failure-case.png

Anyway as it works fine, Moving it to RTBC!

  • lauriii committed e2cb364 on 9.2.x 
    Issue #3180280 by komalk, mherchel, sulfikar_s, ranjith_kumar_k_u,...

lauriii credited jwitkowski79.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented

Committed e2cb364 and pushed to 9.2.x. Thanks!

Leaving open for backport to 9.1.x.

  • lauriii committed 3e30504 on 9.1.x 
    Issue #3180280 by komalk, mherchel, sulfikar_s, ranjith_kumar_k_u,...
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Status: Reviewed & tested by the community » Fixed

Cherry-picked to 9.1.x.

Status: Fixed » Closed (fixed)

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