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

Reference: https://www.drupal.org/core/beta-changes
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
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

DickJohnson’s picture

Investigated 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

Without border

With border

I think the difference on normal desktop, pad or mobile so minimal that my suggestion is to remove the code.

emma.maria’s picture

Title: Clean up "contextual" CSS in Bartik » Clean up the "contextual" component in Bartik
idebr’s picture

FileSize
10.43 KB

I have added before/after screenshots for Windows Firefox and Google Chrome

On the upside:

  • The contextual links display is consistent in the header, footer and content region
  • The border adds a distinction between the underlying text and the button

On the downside:

  • The contextual links button arguably does not look as pretty as it did before

I have no strong feelings either way, so I'll leave this decision to someone else.

idebr’s picture

The contextual link got its current design in #2352857: Improve the display of trigger icon (pencil)

DickJohnson’s picture

FileSize
23.78 KB

I 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.

After

Patch as attachment.

DickJohnson’s picture

Forgot the patch.

idebr’s picture

Status: Needs review » Needs work
FileSize
2.48 KB

Although 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?

DickJohnson’s picture

I 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.

idebr’s picture

Actually, 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:

<div class="contextual">
  <button class="trigger">Open User account menu</button>
</div>

HTML after:

<div class="contextual">
  <button class="trigger trigger--contrast">Open User account menu</button>
</div>

CSS before:

#header .contextual .trigger {
  border: 0;
}

CSS after:

.contextual .trigger--contrast {
  border: 0;
}
DickJohnson’s picture

Yup, that's right. Except that it must somehow been told to target header and footer anyways, right?

emma.maria’s picture

Issue summary: View changes
jp.stacey’s picture

Assigned: Unassigned » jp.stacey

Having a wee look at this now.

jp.stacey’s picture

Assigned: jp.stacey » Unassigned
Status: Needs work » Needs review
FileSize
1.82 KB
1.13 KB

As 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 than Drupal.theme.prototype.contextualTrigger. So do we just overwrite Drupal.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?

Status: Needs review » Needs work

The last submitted patch, 13: clean-up-contextualcss-2398465-13.patch, failed testing.

jp.stacey’s picture

Status: Needs work » Needs review

The 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.

The last submitted patch, 8: clean-up-contextualcss-2398465-8.patch, failed testing.

emma.maria’s picture

Status: Needs review » Needs work

We 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.

#header .contextual .trigger,
.site-footer .contextual .trigger {
  border: none;
}
.contextual-region .contextual .contextual-links a {
jp.stacey’s picture

Assigned: Unassigned » jp.stacey

OK, 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.

jp.stacey’s picture

Assigned: jp.stacey » Unassigned

There 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 in modules/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's header.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 in modules/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!)

emma.maria’s picture

As 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.

  1. Header and footer specific styles for the contextual links were added within this issue... #2352857: Improve the display of trigger icon (pencil). The contextual link design was also tidied up in that issue. The issue was then again worked on after the first commit also and I think the border could of been put back then but it was missed.

    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.

  2.  

  3. The only styles that need to be added by Bartik to contextual links, are styles to override what Bartik sets on links itself. So we need to reset text shadows (because of the Featured Top styles) and reset the borders underneath the links (which is a style set on all links in Bartik).
     
    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.

lauriii’s picture

Status: Needs review » Reviewed & tested by the community

Tested 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.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 9a3ce4d and pushed to 8.0.x. Thanks!

  • alexpott committed b30fc4e on 8.0.x
    Issue #2398465 by DickJohnson, emma.maria, jp.stacey, idebr: Clean up...

Status: Fixed » Closed (fixed)

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