Follow-up to #1342054: [META] Clean up templates and CSS
Problem/Motivation
- Bartik's template files need to be assessed and cleaned up of redundant markup, bad formatting and ID's.
- Bartik's CSS files need to follow Drupal's CSS Coding Standards.
Proposed resolution
For this issue we take "secondary-menu.css" within Bartik in css/components/secondary-menu.css plus any template file associated with the CSS and clean them up.
Remaining tasks
- Write a patch containing as much as the above as possible.
- Post a patch with screenshots.
- Visual review of a patch - check the secondary menu visually with and without patch applied. Take screenshots.
- Code review of a patch - check the code follows coding standards, suggest improvements if needed in a comment.
- Produce a new patch with improvements if needed.
User interface changes
None
API changes
None
Data model changes
None
Beta phase evaluation
Issue category | Task because it is refactoring CSS and templates in Bartik |
---|---|
Issue priority | Not critical because Bartik functions fine we are just doing cleanup tasks |
Unfrozen changes | Unfrozen because it only changes CSS and markup |
Prioritized changes | The main goal of this issue is usability of the Bartik's codebase |
Disruption | non-Disruptive as it is just changing markup and CSS |
Comment | File | Size | Author |
---|---|---|---|
#71 | After patch BT.png | 59.31 KB | Aamir M |
#71 | Before patch BT.png | 70.37 KB | Aamir M |
#70 | Afetr patch code.png | 232.38 KB | bebalachandra |
#70 | After Patch.png | 77.88 KB | bebalachandra |
#70 | Before patch.png | 86.63 KB | bebalachandra |
Issue fork drupal-2502049
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
Comment #1
emma.mariaFrom looking at the secondary-menu.css file I see that we need to include the same refactoring solution that we will add to the Primary menu issue #2502045: Replace the "Primary menu" component with a reusable component in Bartik., which will consider the following...
We should not name CSS selectors based on the location the component is sat within. If someone wants to move/reuse the Secondary menu they can't. Also this method can cause really long over specific selectors that we can improve.
Therefore we need to come up with a solution to be able to apply styles using a resuable component class such as .secondary-menu or .secondary-menu--block (name to be decided based on the component it is added to) instead.
We also need the usual clean up of comment formatting within the CSS file also.
Comment #2
Anonymous (not verified) CreditAttribution: Anonymous at FFW commentedI want try to fix it.
Comment #3
Anonymous (not verified) CreditAttribution: Anonymous at FFW commentedI've looked at this and it seems we need to change menus logic to separate it from region context. May be we need to make separate templates for primary secondary menu - but I don't imagine how will drupal know which menu is it. Can you help me with some advice please.
Comment #4
dcrocks CreditAttribution: dcrocks as a volunteer commentedThis looks like it might be a start, but it definitely needs review. There are differences, but these are based on code in the region component css(sidebars.css in this case) that perhaps shouldn't be there
Comment #5
dcrocks CreditAttribution: dcrocks as a volunteer commentedEven if this patch is acceptable, it might not be possible to do it until other bartik css components are cleaned up. All the region components have a lot of stuff that shouldn't be there. Region components should be for layout and region specific styling but there seems to be a lot of very targeted styling for other components.
Comment #6
dcrocks CreditAttribution: dcrocks as a volunteer commentedComment #7
dcrocks CreditAttribution: dcrocks as a volunteer commentedI forgot color.css.
Comment #8
Manjit.SinghTested Secondary links manually after applying patch.
Please check screenshots of mobile and Desktop.
Comment #9
emma.mariaSetting back to Needs Review as the patch needs a code review also.
Comment #10
emma.maria@dcrocks thanks for the patch!
This issue is very similar to #2502045: Replace the "Primary menu" component with a reusable component in Bartik. so I repeat my comment from there.
The issue we need to solve here is to not rely on the region markup or limit the styles to one particular instance of menu. We want any menu added to this region to have the "secondary navigation" styles. The default provided menu in the region should also not look the same wherever it's placed, the styles should default to basic menu list styling elsewhere.
Therefore we need to add a reusable component class to menu blocks when they are added to the currently named "Secondary menu" region and amend the current CSS to use that class.
Comment #11
emma.mariaComment #12
emma.mariaComment #13
dcrocks CreditAttribution: dcrocks as a volunteer commentedRestart here in same style as #2502045: Replace the "Primary menu" component with a reusable component in Bartik. plus small spaceing change so contextual link wouldn't hide last menu item.
Comment #14
dcrocks CreditAttribution: dcrocks as a volunteer commentedHere is another go. I made these changes:
Modified region--secondary-menu.html.twig as suggested by @laurii
added code to make the title of any block in the secondary menu region hidden
It appears to me that hiding block labels is the design choice for all of the header regions but this may have undesirable results, depending on the block. See attached.
Comment #15
dcrocks CreditAttribution: dcrocks as a volunteer commentedTwo things to note:
1) No content displayed in secondary menu region when logged out, ie, as anonymous user. So it would be unwise to put the user login form in the secondary menu region.
2) There is no code in secondary-menu.css that affects forms, except for block label, so they should display as usual.
Comment #16
dcrocks CreditAttribution: dcrocks as a volunteer commentedTake back some of my comments. Forms do or do not display properly with respect to being logged out vs logged in.
Comment #17
dcrocks CreditAttribution: dcrocks as a volunteer commentedMade some small changes and I think menus display without block labels but everything else still do. Also added images to show differences logged in vs logged out. Still more tweaks needed.
Comment #18
dcrocks CreditAttribution: dcrocks as a volunteer commentedI'd like to see if I'm using the right assumptions for the secondary menu region:
1) All menu blocks have horizontal display and are aligned right (rtl).
2) All other blocks are aligned left (rtl).
3) All labels and text for (a) elements have (near) white color.
4) Block labels are not displayed for menu blocks but are for all others.
I have 2 images that show what it looks like to have most blocks in the secondary menu region. I would like some comment before I make the final changes based on those assumptions.
Comment #19
dcrocks CreditAttribution: dcrocks as a volunteer commentedJust a question. What is the purpose of the colors.css file? It isn't the sole proprietor of 'color:' statements and contains a hodgepoge of code that would make more sense to be with the components actually affected. It doesn't really save that many bytes by combining similar statements in one place but does add the load of an additional file. What component does this file actually correspond too?
Comment #20
dcrocks CreditAttribution: dcrocks as a volunteer commentedComment #21
dcrocks CreditAttribution: dcrocks as a volunteer commentedLooking at #10 again, I have probably done too much. If that is so, I can supply a menu only patch(#14?) and not worry about other blocks being placed in the secondary menu region.
Comment #22
dcrocks CreditAttribution: dcrocks as a volunteer commentedThis is a slightly tweaked version of #14 that only affects menus that might be placed in the secondary menu region. The image shows if user account, main menu, tools menu, and footer menu were all in the secondary menu region.
Comment #23
dcrocks CreditAttribution: dcrocks as a volunteer commentedHere is diff as well
Comment #24
emma.mariaHi @dcrocks I posted a reply to your same question about colors.css here.
Comment #25
emma.mariaThanks @dcrocks for working on this issue.
I applied the patch in #22 and found no visual regressions.
The Twig template looks great apart from the extra line space at the bottom of the file.
I noticed some improvements for the CSS.
Why was the margin changed to 23px? Also can it please be changed to 15px to be consistent with the layouts of all of the other regions.
And can the margin please be moved to the
.layout-secondary-menu
selector, it is layout styles for the region.We can remove the
margin: 0
it is already declared by.menu-item
.We shouldn't purposively hide things via theme CSS, please remove this code. The block title is also already hidden by the visually-hidden class also.
Comment #26
emma.mariaComment #27
HOG CreditAttribution: HOG at Skilld commentedAdded changes to #22 patch.
Comment #28
dcrocks CreditAttribution: dcrocks as a volunteer commented@emma.maria
#1: I changed this because the contextual help icon floats on top of the menu, obscuring it from view.
#3: I thought that we needed to make it so any menu added to the secondary menu region would have the same appearance as the user account menu. Most other menus do not have the menu label turned off by default as the user account and main menu do.
Comment #29
dcrocks CreditAttribution: dcrocks as a volunteer commentedThis is what it looks like to add other menus(Tools, Main) to the secondary menu region in a current clone of 8.0.0. As you can see the block label for the Tools menu block is displayed on a separate line from the rest of the menu. This doesn't fit the styling for the secondary menu region(or the primary menu region). I thought from #10
we needed to make them match style. There may be other, programmatic, ways than css to remove labels from menu blocks when in the secondary or primary menu regions and I will look for them if that is what you would prefer. But I would mention the "visually-hidden" css is used in other places in bartik.
Comment #30
emma.mariaWe shouldn't tackle that in this issue, this is a code cleanup issue only. This other issue handles that topic #2355501: Contextual link triggers cover too much of small contextual regions.
For
Yes it is used in other places in Bartik but we are actively trying to remove them amongst the clean up issues. The footer no longer has this behaviour, we haven't got to the header part of the site yet.
If the title shows for any further menu blocks added the user can toggle the display of the title through the UI which is expected behaviour for blocks.
Comment #31
dcrocks CreditAttribution: dcrocks as a volunteer commentedActually a user can toggle 'Display Title' on or off only thru the 'Place Block' forms. If a user simply 'moves' an existing block from one region to another, there is no option to change it. Might not be a big deal but still another UI mystery.
Comment #32
emma.maria#31 erk that is not a not fun UI bug.
I am going to review the code in #27, thanks @HOG for the patch.
Comment #33
Bojhan CreditAttribution: Bojhan as a volunteer commentedI discussed this with emma.
We should be supporting adding menu's to this region. We shouldn't special case block titles, or other block elements - as the primarily purpose is for menu's. The design doesn't support this.
Comment #34
dcrocks CreditAttribution: dcrocks as a volunteer commentedA reroll
Comment #35
dcrocks CreditAttribution: dcrocks as a volunteer commentedReroll because of #2503755: Switch from user login block to login menu link and search block in standard profile
Comment #36
dcrocks CreditAttribution: dcrocks as a volunteer commentedComment #37
dcrocks CreditAttribution: dcrocks as a volunteer commentedThis still applies and could use some attention.
Comment #38
dcrocks CreditAttribution: dcrocks as a volunteer commentedreroll
Comment #39
emma.mariaYes this does needs attention :) However this work is now for the 8.1.x branch as this is an improvement to Bartik and not a bug, the patch still applies cleanly.
Comment #40
emma.mariaI take that back I spoke to @davidhernandez. Anything that requires a change record should be 8.1 only everything else for Bartik can stay in 8.0.x.
Comment #46
kaythay CreditAttribution: kaythay at Aten Design Group commentedNo changes made, just reroll.
Comment #53
Sakthivel M CreditAttribution: Sakthivel M at QED42 for Drupal India Association commentedComment #54
Sakthivel M CreditAttribution: Sakthivel M at QED42 for Drupal India Association commented#54 Please review the patch
Comment #55
Rinku Jacob 13 CreditAttribution: Rinku Jacob 13 at Zyxware Technologies commentedpatch#54 applied successfully for drupal 9.3.x-dev.but the menu is not visible as much as.Adding screen shot for the reference
Comment #56
chetanbharambe CreditAttribution: chetanbharambe at QED42 for Drupal India Association commentedVerified and tested patch #54.
Patch applied successfully but not working as expected.
Issue Description: Replace the "Secondary menu" component with a reusable component in Bartik
Actual Results:
# Secondary menu is not visible cleanly after applying the patch.
# Secondary menu aligned as vertically (should be aligned horizontally)
Please check the attached screenshot.
Moving to needs work.
Comment #57
Sakthivel M CreditAttribution: Sakthivel M at QED42 for Drupal India Association commented#57 Please review the patch
Comment #58
Asha Nair CreditAttribution: Asha Nair commentedI have applied patch #57 for drupal 9.3.x-dev. But I can't see any changes.
Comment #59
chetanbharambe CreditAttribution: chetanbharambe at QED42 for Drupal India Association commentedHi @Sakthivel M,
Thanks for patch #57
The patch is applied successfully but not working as expected
Please check the comment mentioned in #56 (Actual Results)
Currently, As a user, I am not able to see any changes after applying patch #57
Please check the attached screenshots.
Moving to needs work.
Comment #62
Shubham Chandra CreditAttribution: Shubham Chandra as a volunteer and at Dotsquares Ltd. commentedRerolled patch against #57 in 9.5.x
Comment #65
pradhumanjain2311 CreditAttribution: pradhumanjain2311 at OpenSense Labs for DrupalFit commentedFix CS error by removing space.
Comment #66
Aamir M CreditAttribution: Aamir M at QED42 for Drupal India Association commentedComment #67
Aamir M CreditAttribution: Aamir M at QED42 for Drupal India Association commentedPatch #65 was applied successfully but the Secondary menu is not visible properly after applying the patch.
Even when you hover the mouse over a link (i.e My account or Log out) then the link is not properly visible.
Screenshots are attached for reference
Hence moved to needs work
Comment #68
sahil.goyal CreditAttribution: sahil.goyal as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedHi, Providing the updated patch as per considering the issue about Bartik's CSS files need to follow Drupal's standard. As well as per the #67 comment that Secondary menu is not visible properly after applying the patch it move the the left, color got detach and inline css left over. So here is the updated Patch.
Comment #69
sahil.goyal CreditAttribution: sahil.goyal as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedUploading the intradiff file as missed
Comment #70
bebalachandra CreditAttribution: bebalachandra as a volunteer and at Srijan | A Material+ Company for Drupal India Association commented#68 patch applied successfully on Drupal 9.5. Issues stated in #67 resolved.
Keeping this issue in needs review for further discussions.
Adding screenshots for reference
Comment #71
Aamir M CreditAttribution: Aamir M at QED42 for Drupal India Association commentedPatch #68 was applied successfully and also when the mouse hovers over the My Account/Logout link then it's properly visible
Screenshots are attached for the reference
Looks good to me
Hence moved to RTBC
Comment #72
bnjmnmWhere/when does the .layout-secondary-menu class appear?
The issue summary states this is for cleaning up the CSS. This is changing the margin amount. Not clear what the intent is here. Perhaps it's evident by reading through all the comments, but an issue summary update would be preferable
In general it isn't clear how the reported issue translates into the patch that was RTBCd.
It's also worth acknowledging that Bartik is deprecated in 9.5. This is the last version of Drupal core it will appear in. If this isn't a bug fix it may not be worth addressing (the only bugs I can see are ones introduced by earlier iterations of the patch).
Comment #73
Feuerwagen