Problem/Motivation
Follow-up discovered while testing #2405059: Replace menu images with Libricons.
A menu in the sidebar configured to display expanded sub-menu items does not properly indent.
Steps to reproduce
1) Modify the Home link in the main menu to "Show as Expanded".
2) Create a Basic Page, and add it as a sub-menu link of the Home page in the main menu.
3) Move the main menu out of the Header region, into one of the Sidebar regions of Bartik theme.
4) Configure the Menu Block to display an unlimited number of levels.
EDIT Jordana:
Simpler Steps to reproduce:
- Install Drupal 8.4.x
- Add a few pages and create the menu items for them in a hierarchy:
- Go to nod/add/page
- On the right open Menu Settings and create a menu
- Set the menu parent to another page (such as the page you created before
- Go to the Block Page (admin/structure/block) and add the Main Navigation to one of the sidebars(ie Sidebar First)
- See that the submenu's are not indented in the Sidebar
Proposed resolution
Add indentation for sub-menus using CSS margins.
Remaining tasks
Write a patch.
Screenshots.
User interface changes
Sub-menu items will be displayed hierarchically using indentation
(edit Jordana: by using 1em so the arrows align)
API changes
None.
Data model changes
None.
Comment | File | Size | Author |
---|---|---|---|
#79 | Screenshot 2017-04-28 10.54.43.png | 27.53 KB | mikeohara |
#59 | 2607210-59-after-patch.png | 8.56 KB | jordana |
#59 | 2607210-59-before-patch.png | 8.48 KB | jordana |
#59 | 2607210-59.patch | 475 bytes | jordana |
#52 | alignment.png | 14.54 KB | Manjit.Singh |
Comments
Comment #2
star-szrCan you reproduce this with Stark or Classy? If not it might be specific to Bartik.
Comment #3
EdgarPE CreditAttribution: EdgarPE commentedYes, it's Bartik specific. Classy shows sub-items indented correctly.
Comment #4
EdgarPE CreditAttribution: EdgarPE commentedComment #5
Shabbir CreditAttribution: Shabbir at Trigyn Technologies Ltd commentedComment #6
Shabbir CreditAttribution: Shabbir at Trigyn Technologies Ltd commentedProviding a patch to fix the issue.
Comment #7
ameymudras CreditAttribution: ameymudras as a volunteer commented@Shabbir, just tested the patch provided by you. The margin would also apply to the parent ul tag which is not required. Providing a specific one.
Comment #8
Nitesh Pawar CreditAttribution: Nitesh Pawar at Trigyn Technologies Ltd commentedI applied the patch and it works fine. I have uploaded screen shot for the same.
Comment #9
laxman.ghavte CreditAttribution: laxman.ghavte at Faichi Solutions Pvt Ltd commentedAfter patch apply it works fine
Comment #11
ameymudras CreditAttribution: ameymudras as a volunteer commentedTest fails were entirely unrelated
Comment #12
Shabbir CreditAttribution: Shabbir at Trigyn Technologies Ltd commentedComment #13
joelpittetAnd this looks like it needs RTL support. with [dir="rtl"] RTL attribute selector.
Nit picks space before the starting curly bracket, 2 space intent, and ending new line.
Comment #14
ameymudras CreditAttribution: ameymudras as a volunteer commentedThanks joelpittet, have attached a inter-diff for the indentation. Regarding the RTL support i think it is already taken care of by bartik (please see the screenshot).
Please let me know in case i am missing something
Comment #15
joelpittet@ameymudras thanks for the interdiff, that's looking better. You may have forgot the patch in #14.
Regarding RTL, if you are adding styles that are targetting left or right margin/padding you should think how that will be represented in LTR. If it's already taken care of in RTL like that then it should be fine in LTR without this patch fix.
Or in other words, it should look like the before and after shots in #8 for RTL just the same as LTR.
Comment #16
prateekS CreditAttribution: prateekS commentedComment #17
prateekS CreditAttribution: prateekS as a volunteer commentedI have tested with some minor modification in menu.css and thats working fine for me and both RTL, LTR.
Attaching patch file for it. Please review it.
Comment #18
prateekS CreditAttribution: prateekS as a volunteer commentedComment #19
prateekS CreditAttribution: prateekS as a volunteer commentedComment #20
prateekS CreditAttribution: prateekS as a volunteer commentedComment #21
imalabya+.menu-item--expanded ul.menu{
This doesn't meet our CSS coding standards: https://www.drupal.org/node/1887862
Also there was an extra new line in the end of file.
Comment #22
joelpittetThe RTL needs to be addressed still, see #15.
Comment #23
kostyashupenkoMenu looks ok on my side for rtl & ltr. What need to fix?
Comment #24
imalabyaYes, the before and after for the menu should look same for both RTL as well as LTR.
After removing the changes in the patch I found that the contents in the menu.css doesn't quite effect the menu blocks.
Adding a screenshot of before and after of both RTL and LTR along with the updated patch.
NB: I have kept the bottom padding intact as it is for the theme looks.
Comment #25
Thirugnanam CreditAttribution: Thirugnanam as a volunteer and commentedTested by applying the patch in simpletest.me and its working.
Comment #26
Saphyel CreditAttribution: Saphyel as a volunteer commentedTested on my local and works as #25
Comment #27
alexpottWe need to work out why #2548805: Add sensible base layout styles for lists in Bartik. added this. Has this caused visual regressions somewhere else? I suggest also reaching out to @emma.maria in case she remembers why #2548805: Add sensible base layout styles for lists in Bartik. did what it did.
Comment #28
timisoreana CreditAttribution: timisoreana at Skilld commentedWas tested LTR and RTL.
Actual result: Identation on RTL menu is bigger than on LTR menu
Expected result: Identation on RTL and LTR menu should be the same
Comment #29
timisoreana CreditAttribution: timisoreana at Skilld commentedComment #30
emma.mariaCore list styling was causing issues in Bartik. It was decided to add a sensible reset to help with list styling that affected so many different kinds of lists.
It was never the intention to disrupt indentation.
I will take a look at this issue myself.
Comment #31
emma.mariaUpdating issue title and priority.
Comment #33
idebr CreditAttribution: idebr at One Shoe commentedI closed #2682307: Expanded Menu Items should indent sub-tree as a duplicate of this issue.
Comment #34
emma.mariaI quite like the solution in #2682307: Expanded Menu Items should indent sub-tree as it doesn't take away the styles that we needed to add for Bartik for lists.
I will ask @jwilson3 to post his patch into this issue instead.
Comment #35
jwilson3Patch from Issue #2682307. Kudos to @Emma.marie for finding my issue and linking the two! That almost never happens :P
This issue is specific to the Bartik theme, where there is a CSS override that sets the margins on all
ul.menu
to 0, so the easy fix is to override that override for the specific case of sub-menu trees.This patch from #2682307: Expanded Menu Items should indent sub-tree does exactly that.
Comment #36
joelpittetThanks @jwilson3 that looks much more like a solution for both RTL and RTL. Just tagging for the DrupalCon Sprints. Thanks for the previous manual testing @timisoreana!
If it is the same for both RTL and RTL with manual testing I'd say this is RTBC (reviewed and tested by community)
Comment #37
emma.mariaComment #38
bronzehedwickTested the patch, and it applied cleanly and fixed the regression.
Comment #39
star-szrLooks good just has some standards issues.
IMO this comment is unnecessary, the CSS below explains this with the expanded selector and the margin. If we keep it, it should end in a period per https://www.drupal.org/node/1354#drupal.
This line is missing the LTR comment per https://www.drupal.org/node/1887862#comments - "Styling for Right-To-Left Languages".
Comment #40
jwilson3Comment #41
jwilson3Thanks @Cottser. This patch fixes #39.1 and #39.2 I originally wanted to fix #39.1 by removing the comment as @Cottser suggested, however after reading through the docs about Blank lines (https://www.drupal.org/node/1887862#blank-lines) I understand that I would have also had to remove the blank line that separates these two new rules from the previous section which IMO seemed to not flow correctly when looking at the entire file. Therefore I fixed #39.1 by just adding the period. Hope this makes sense.
Comment #42
jwilson3Comment #43
star-szr@jwilson3 yup that makes sense, thanks!
Comment #44
star-szrSorry to kick this back after RTBCing it, after manual testing I think we are not quite restoring things to how they were pre-regression.
From what I can see the spacing on the left was previously 1em, not 1.5em. The screenshots I'm attaching aren't an apples-to-apples comparison but hopefully you can see the difference. The key thing to look for is the arrows should line up with the text.
The before screenshot was taken on commit 70ecd27, one commit before #2548805: Add sensible base layout styles for lists in Bartik..
70ecd27 pre-regression:
8.2.x + #41:
Comment #45
emma.mariaThanks @Cottser. I'm going to take a closer look at this during the sprint at Frontend United this weekend, which you will be at too :-)
Comment #46
Maninders CreditAttribution: Maninders at Srijan | A Material+ Company commentedPatch #41 is working fine, but there is 1 issue in RTL design, so i fixed it with the padding-right:0. Please test the patch, and screenshots are attached as well.
Comment #47
star-szr@Maninders thanks that seems like a good change. Putting back to needs work so #44 can be addressed.
Comment #48
Maninders CreditAttribution: Maninders at Srijan | A Material+ Company commentedComment #49
Maninders CreditAttribution: Maninders at Srijan | A Material+ Company commentedComment #50
Maninders CreditAttribution: Maninders at Srijan | A Material+ Company commented@Cottser as per your comment #44, the css code (left space reduce from 1.5em to 1em on LTR & on RTL Right space reduce from 1.5em to 1em.) is updated. Please check the screenshots and review the patch.
Comment #51
star-szr@Maninders thanks! I know it's a small patch we're working on but it's good to get into the habit of doing interdiffs :) they are really helpful.
Code-wise this looks good. Needs another round of manual testing and screenshots so leaving at needs review.
Comment #52
Manjit.Singh#50 looks good on RTL and LTR.
currently, arrows are not align with the content, I don't know if it is a part of design but can we do something like this so that arrows and content should be in one line.
Comment #53
star-szr@Manjit.Singh if I understand correctly that alignment you're talking about is how it was pre-regression so probably in scope here (see my comment in #44 with pre- and post-regression screenshots for a clearer example of the difference in overall alignment).
Personally I generally like to align text with other text ("hanging" bullets and other marks in the margin) instead of aligning text to bullets or arrows but we should probably go with what Bartik has established as a pattern for the purpose of resolving this. Sending over to our Bartik maintainer for her feedback on this point :)
Comment #54
imalabyaComment #55
star-szrAt this point we probably just want things to look like the "pre-regression" alignment as shown in #44. That's the simplest and clearest path forward I think.
Comment #56
paintingguy CreditAttribution: paintingguy commentedBartik patches (please update this in the patches for Drupal 8)
I've been having to a patch Bartik since Drupal 8 was launched, yet this patch has never been updated to the patches.
Is there any chance this could finally be put to rest? It's so annoying having to do these manually (especially when they have been acknowledged and resolved) every time a new release comes out. Thank you for your help.
This is what I add to the menu.css
diff --git a/core/themes/bartik/css/components/menu.css b/core/themes/bartik/css/components/menu.css
index efde5d2..95420de 100644
--- a/core/themes/bartik/css/components/menu.css
+++ b/core/themes/bartik/css/components/menu.css
@@ -5,7 +5,7 @@
Comment #59
jordanaSo I'm coming in from a duplicate issue #2794565: Multi-level menu block styling in Bartik is not great
To me it makes more sense just to add the 1em left padding to the already set "padding" style in the menu.css
Before:
core/themes/bartik/css/components/menu.css
After:
core/themes/bartik/css/components/menu.css
To me this seems a cleaner solution than adding a margin-left or padding-left etc etc.
This also addresses @cottser in #2607210-44: Visual regression: Sidebar menus are missing indentation styles. - the arrows line up with the text as it is 1em padding.
See my findings in this comment:
#2794565-10: Multi-level menu block styling in Bartik is not great Which basically shows that there is a padding:0 added to LTR which messes up indentation.
And this will fix the issue in both LTR and RTL, my patch and before and after pics for reference - 2607210-59.patch
EDIT: Tested Patch on 8.3.x and 8.4.x and patch fixes issue on both
RTL before after testing:
Before RTL
After RTL
Comment #60
jordanaSImpler Steps to reproduce:
-- edited the Issue summary
Comment #61
jordanaComment #62
jordanaComment #63
jordanaComment #64
jordanaComment #65
jordanaComment #66
paintingguy CreditAttribution: paintingguy commentedThis is what I am using to patch this since the launch of Drupal 8.
Comment #67
star-szr@jordana that does indeed look much cleaner, thank you!
Comment #68
star-szrUnassigning to see if we can get some folks to review this.
Comment #69
jordana@cottser - glad to help! Thanks for all your time :) I'm sorry to have missed you at #fldc17
@paintingguy - did you try out the patch? I understand that adding the margin-left works, but it only works on LTR and adds extra code. adding the left padding to the already available and set style in
is imho cleaner BUT apart from that, it reverts it back to what it actually was set to in bartik/css/base/elements.css:
So according to what @cottser stated in one of the comments above - ideally we would like it to be reverted back to working order pre-regression and without adding extra code. Looking at Bartik's elements.css - the left padding of 1em in LTR and right 1em padding in RTL is what was intended to be used. Again please see my comment in the duplicate issue for a more in depth explanation - #2794565-10: Multi-level menu block styling in Bartik is not great
So @paintingguy - if we'd like to get this fixed asap - testing the patch and letting us know here in the comments what you results were, would greatly help in getting this commited.
Thanks!
Comment #72
star-szrMaking good on what I said in #2794565-17: Multi-level menu block styling in Bartik is not great by adding credits.
Comment #73
star-szr@jordana - sorry we missed each other in Florida! Thanks for your thorough work on this issue :)
Comment #74
paintingguy CreditAttribution: paintingguy commented@jordana
sorry for the delay.I'm very happy this is finally being looked at.
I must apologize as I am a novice at any testing or understanding how you all test code.
What exactly would you like me to do?
Comment #75
jordana@paintingguy
Basically we'd like you to reproduce the steps; checkout comment #60 - #2607210-60: Visual regression: Sidebar menus are missing indentation styles. or the issue summary I edited.
You would install Drupal 8.4.x, set up the multilevel menu, place it in a block and do a before and after of the patch (preferably with screenshots if possible).
Let me know if you need more info or help and I could maybe walk you through it on Slack, irc or skype.
Thanks so much for your time and help!
Comment #76
star-szrThis was discussed with some of the other core committers and we agreed to downgrade this to a normal bug. It is a visual regression and under the normal bug criteria the following is listed:
Thanks to those who have triaged and worked on this issue recently and not-so-recently. Let's try to keep this moving and get it resolved, I think we are closer than ever!
Comment #77
mikeoharaI am picking this up as part of the Friday Sprints at DC2017
Comment #78
jordanaHey Mike!
That's great! Basically all that needs to be done is that it needs to be tested so it can be marked RTBC
It would be great to finally get this done, so thanks so much!
Comment #79
mikeoharaI was able to test and confirm using steps from #60 and the confirmed patch, and this is working as expected.
Comment #80
star-szrThanks! I should be able to review this one today.
Comment #81
mikeoharaGreat! Should it be bumped to 8.4.x?
Comment #84
star-szrI tested this on LTR and RTL and it looks great. I reviewed the CSS again and it makes sense and is a nice minimal change :)
Thanks everyone! Setting the version to 8.3.x-dev because I backported the change there since it's a non-disruptive bug fix to Bartik, an internal theme.
Committed and pushed 1e04b3623f to 8.4.x and b4b87ff4da to 8.3.x from DrupalCon Baltimore. Thanks!