Problem/Motivation
Bartik's code needs to meet current Drupal coding standards.
Proposed resolution
This issue takes a specific section of Bartik's code, it acts as a "component" issue and improves that section of code with minimal impact on the rest of Bartik's codebase.
This issue aims to clean up and properly format the CSS and templates files without breaking Bartik visually.
This issue primarily looks at the css/components/contextual.css
file.
Work that needs to be included in patches for this issue are fully outlined in the META issue #1342054: [META] Clean up templates and CSS.
Remaining tasks
- Write a patch
- Review the patch - code review and visual changes
- Upload screenshots to show nothing or something is broken on the frontend
User interface changes
We intend to keep Bartik's overall design and feel the same but we are using component based practices. The contextual links are a standalone component and should aim to have the same styles regardless of where they are.
API changes
n/a
Beta phase evaluation
Issue category | Task because it is a code clean up overhaul of a theme. |
---|---|
Unfrozen changes | Unfrozen because it only refactors CSS and templates, no changes to UI or APIs |
Prioritized changes | The main goal of this issue is usability and performance. We want Bartik code to be up to date and something to be proud of. |
Disruption | No disruption it's only refactoring code not changing how to use the theme |
Comment | File | Size | Author |
---|---|---|---|
#20 | cleanup-contextual-links-bartik-2398465-20.patch | 643 bytes | emma.maria |
#20 | footer-contextual-after.png | 10.25 KB | emma.maria |
#20 | header-contextual-after.png | 15.77 KB | emma.maria |
#20 | header-contextual-before.png | 13.35 KB | emma.maria |
#20 | footer-contextual-before.png | 10.83 KB | emma.maria |
Comments
Comment #1
DickJohnson CreditAttribution: DickJohnson commentedInvestigated this a bit.
On contextual.css we're changing contextual filters trigger style on header and footer. Difference is relatively minimal and it even looks a bit like a bug (it should either be completely without borders or completely with borders, right?). As an attachament few images of contextual filter in header. On first one it's like it is now and on second one without stuff on current contextual.css
I think the difference on normal desktop, pad or mobile so minimal that my suggestion is to remove the code.
Comment #2
emma.mariaComment #3
idebr CreditAttribution: idebr commentedI have added before/after screenshots for Windows Firefox and Google Chrome
On the upside:
On the downside:
I have no strong feelings either way, so I'll leave this decision to someone else.
Comment #4
idebr CreditAttribution: idebr commentedThe contextual link got its current design in #2352857: Improve the display of trigger icon (pencil)
Comment #5
DickJohnson CreditAttribution: DickJohnson commentedI think it also looks better without the border, but I'm not sure how important it is anyways. However, currently the border is just removed partially so it should be removed fully.
Before image is like the first image on #1. Here it is like it's afterwards.
Patch as attachment.
Comment #6
DickJohnson CreditAttribution: DickJohnson commentedForgot the patch.
Comment #7
idebr CreditAttribution: idebr commentedAlthough it does look prettier in the header, the contextual links on a white background can no longer be discerned from the background:
tkoleary who designed the contextual links explains his take on the buttons at https://www.drupal.org/node/2352857#comment-9237679
Considering the implications of changing the visual style for the contextual component, perhaps we should leave it as is and focus on refactoring the css instead?
Comment #8
DickJohnson CreditAttribution: DickJohnson commentedI thought that original issue summary said that if something weird is noticed it can be changed/fixed and from my point of view a block having border just partially was one of those. After reading the explanation at #2352857: Improve the display of trigger icon (pencil) I agree with you and let's keep this just as css-refactor.
This is the kind of issue where I'm thinking if we should actually postpone this until footer and header components have been cleaned up, instead of starting to fix the templates. But a patch removing border-changing things from #6 as attachment.
Not setting it as need of review by purpose.
Comment #9
idebr CreditAttribution: idebr commentedActually, we don't necessarily have to wait until the header and footer have been refactored. In SMACSS world, you would avoid re-styling components based on their location, and instead use variants. Maybe it would make sense to do the same here.
For the contextual links in the header and footer, we could add a class '
trigger--contrast
' to the link and apply to styling to that instead. This makes the contextual component reusable for other regions as well.HTML before:
HTML after:
CSS before:
CSS after:
Comment #10
DickJohnson CreditAttribution: DickJohnson commentedYup, that's right. Except that it must somehow been told to target header and footer anyways, right?
Comment #11
emma.mariaComment #12
jp.stacey CreditAttribution: jp.stacey at Magnetic Phield commentedHaving a wee look at this now.
Comment #13
jp.stacey CreditAttribution: jp.stacey at Magnetic Phield commentedAs far as I can see, the existing patch in comment #8 has been already implemented in 5a46466: #2398445: Clean up the "elements" component in Bartik.
Tackling @idebr's comment #9.... To add this new class involves custom Drupal.theme Javascript. This seems to have changed in D8, although the docs I've found still only cover 6.x/7.x.
I note that core/modules/contextual/js/contextual.js defines
Drupal.theme.contextualTrigger
rather thanDrupal.theme.prototype.contextualTrigger.
So do we just overwriteDrupal.theme.contextualTrigger
? On that basis, a simple patch to add the trigger--contrast class is attached (no interdiff, given the previous patch is now obsolete.)However, the problem this class is trying to solve (#7) seems OK now in HEAD: I've attached a screenshot to compare with 2398465-6-after.png:
4 months ago, after patch:
Now, no patch:
Thoughts? Is there in fact anything left to do on this issue?
Comment #15
jp.stacey CreditAttribution: jp.stacey at Magnetic Phield commentedThe patch failed because there's apparently a requirement that anonymous users get no Javascript from core. There's probably a best-practice way to get the js: out of base: in libraries.yml and into another component, but I'll only work on it if it's actually required for this ticket!
I still think we're done here and this ticket should be resolved without any patch, but please correct me if that's wrong.
Comment #17
emma.mariaWe still need to work on the contextual.css file. There is one ID in there and some long gnarly CSS selectors we need to deal with.
Comment #18
jp.stacey CreditAttribution: jp.stacey at Magnetic Phield commentedOK, just to confirm, then:
* We don't need the patch to contextual.css on #8 as its meat was implemented in 5a46466.
* We don't need my suggestion on #13 as the issue described in #6/#9 has been fixed (probably in core contextual.css, but fixed nonetheless).
Is that right? I'll look at contextual.css in the light of #17 now, but it's a pretty short file.
Comment #19
jp.stacey CreditAttribution: jp.stacey at Magnetic Phield commentedThere are literally three rules in
contextual.css
. I list them below with any proposed changes but could do with advice about them, please!1.
#header .contextual .trigger, .site-footer .contextual .trigger
This needs some kind of specifier to override
.contextual .trigger
inmodules/contextual/css/contextual.icons.theme.css
, for header and footer.*
.site-footer
is the best candidate for the latter*
#header
(matching <header id="header" ...>) could be replaced by a number of candidates: I see Bartik'sheader.css
uses[role*=banner]
: is that preferred to e.g..header
?2.
.contextual-region .contextual .contextual-links a
This has to override
.contextual-region .contextual .contextual-links a
inmodules/contextual/css/contextual.theme.css
and so can't be any shorter. I think we should leave this as it is.3.
.contextual-links
As this is very low specificity, I would normally put it higher up in the file to indicate to the reader that it could affect later rules, but I'm happy to leave it where it is if that's preferred.
(That's it: advice, pls!)
Comment #20
emma.mariaAs said in #18 the patches so far in the issue are not needed. However I see the following work for contextual links still needed, and as this issue has not been moved for a while I created the fresh patch.
I currently see no issue with allowing the border on the links to show within the header and footer. See my screenshots below:
Before:
After:
The contextual links should definitely aim to look the same no matter where they are displayed on the page.
All other styles are handled by the contextual links module and do not need to be set again in Bartik, they were already being declared by the module.
Also I also simplified the CSS selector used within Bartik as it did not need to match the Contextual links module with it's crazy specificity. It just had to be specific enough to override the basic link selector plus target contextual links as simple as possible.
To review this issue we need screenshots of the links within contextual links for as many regions in Bartik as possible. This is to show no visual regressions, however as mentioned above the border around the trigger in header and footer is accepted.
Comment #21
lauriiiTested this manually and found out that after this font size is a little bit bigger (consistency yay!) and border is being added for the trigger element in header and footer. Didn't find any places where contextual links would be broken; the changes are quite simple so that was expected.
Comment #22
alexpottCommitted 9a3ce4d and pushed to 8.0.x. Thanks!