Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplify #geolocate_link styling #3006

Merged
merged 2 commits into from May 15, 2020

Conversation

zarino
Copy link
Member

@zarino zarino commented May 1, 2020

Fixes mysociety/fixmystreet-commercial#1835.

Reduces the amount of styling on the #geolocate_link in core, because we’re seeing more cobrands now (eg: bexley, bristol, bucks, lincs) where the very opinionated "dark grey button with rounded bottom corners" styling just didn’t fit and had to be overwritten, separately, again and again.

Rather than keep the grey button styling in just a few cobrands, I‘ve removed it everywhere. Also took the opportunity to increase the font size, so it’s a bit more tappable on touchscreens.

Before, and after, at narrow widths:

fmscom

island-roads

highways

While I was at it, I also reduced the specificity of all the #geolocate selectors (in core and the cobrands). No need for them to be as specific as they were.

@codecov
Copy link

codecov bot commented May 1, 2020

Codecov Report

Merging #3006 into master will decrease coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3006      +/-   ##
==========================================
- Coverage   83.07%   83.07%   -0.01%     
==========================================
  Files         246      246              
  Lines       15429    15429              
  Branches     2886     2886              
==========================================
- Hits        12818    12817       -1     
  Misses       1695     1695              
- Partials      916      917       +1     
Impacted Files Coverage Δ
perllib/Utils.pm 97.97% <0.00%> (-1.02%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 66bcc3b...5bf3d1b. Read the comment docs.

@zarino zarino changed the title [WIP] Simplify #geolocate_link styling Simplify #geolocate_link styling May 1, 2020
@zarino zarino requested a review from dracos May 1, 2020 14:00
@dracos
Copy link
Member

dracos commented May 7, 2020

Do you think it's obvious enough as a link? Feels like use my location is more a button-y thing than a link-y thing maybe and as there are no other links in view I wouldn't even realise it was a link. Dunno.

Why so many override default #front-main a` - why is there a default colour (inherit) set for that at all?

@dracos dracos marked this pull request as ready for review May 7, 2020 13:17
@zarino
Copy link
Member Author

zarino commented May 11, 2020

Do you think it's obvious enough as a link? Feels like use my location is more a button-y thing than a link-y thing maybe and as there are no other links in view I wouldn't even realise it was a link. Dunno.

Yeah, I’m not sure this will be the "final" style for this button, but at least this PR makes the button easier to style in future. I think the link styling will do for now, while we have discussions about all the other changes we want to make to this form. Happy to be persuaded otherwise though, if you feel strongly against it.

Why so many override default #front-main a` - why is there a default colour (inherit) set for that at all?

A good question. I assume it was there because some cobrand(s) at some point wanted to include links above the search box, and the default blue link colour didn’t have good enough contrast with the #front-main background colour. My instinct is we could just remove it, since it seems to be causing more issues than it solves right now. There’s a ticket for "considering" what to do about it: #3007.

@dracos
Copy link
Member

dracos commented May 12, 2020

Could we underline it at least? On mobile, I'm just not sure how you'd tell the difference between it and any other text (not saying the old one was brilliant or anything, but it does at least usually look a bit like the main form "Go" button...).

Sorry, had missed #3007.

I note when you click the geolocation link, before it finishes, it gets a black background inherited from base, which would be good to fix as part of this! You did, my style wasn't update, ignore me.

Copy link
Member

@dracos dracos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple of tiny points.

web/cobrands/sass/_layout.scss Outdated Show resolved Hide resolved
web/cobrands/tfl/layout.scss Show resolved Hide resolved
Much less opinionated styling for #geolocate_link in core, which
means the link should look better, by default, for most cobrands.

(In particular, the link looks much better on cobrands with the more
modern style of white / off-white #front-main background, such as
highwaysengland, which was the cobrand that started off this work
to begin with.)

I’ve also reduced the specificity of the #geolocate_link rulesets
in both core and all the cobrands.

While this commit means that fewer cobrands need to override styles
on #geolocate_link, there are still 8 cobrands that have to define a
custom text colour for their #geolocate_link, because they both:
A) have a light coloured background for #front-main, and
B) want their #geolocate_link to be coloured like a normal link,
rather than inheriting the text colour of the parent element.

We might want to revisit this handling of #geolocate_link colouring and
the `#front-main a { color: inherit }` rule in _layout.scss at some
point in the future.

Fixes mysociety/societyworks#1835.
@zarino zarino force-pushed the issues/commercial/1835-geolocation-button-styling branch 2 times, most recently from d6e3b86 to 4331d90 Compare May 15, 2020 08:48
The colour and text decoration of links inside `#front-main` can now
be customised via `$primary_link_*` Sass variables.

Text decoration is set in _base.scss (like global link text decoration)
while colour is handled in _layout.scss (which is where `#front-main` is
given its background colour, so likely also the time you’ll want to set
a contrasting colour for links inside it).

The colour variables are set to `null` by default, meaning that no
colour or rules for those links will be compiled, enabling the links to
inherit the global link colour style, which previously wasn’t possible.

The decoration variables are set to `underline` by default, because most
cobrands set their #front-main links to be the same colour as the
surrounding text, so adding an underline is a sensible default. You can
disable the underlines by setting `$primary_link_decoration: none` in
your cobrand’s _colours.scss file, as cheshireeast and oxfordshire do.

Fixes #3007.
@zarino zarino force-pushed the issues/commercial/1835-geolocation-button-styling branch from 4331d90 to 5bf3d1b Compare May 15, 2020 08:57
@zarino
Copy link
Member Author

zarino commented May 15, 2020

Ok @dracos:

  • Underlined links in #front-main is now the default, with cheshireeast and oxfordshire overriding $primary_link_decoration to none – they can’t use null because then the underline !default from base.scss would get applied.
  • Links now have an underline in tfl 👍
  • $primary_link_hover_decoration is now set to the value of $primary_link_decoration by default (not a big change, but seemed like it would save people some hassle if they, like cheshireeast, want the links to always be undecorated).
  • Un-used $primary_link_decoration and $primary_link_hover_decoration removed from _layout.scss.
  • Issue numbers now in Changelog.
  • I haven’t squashed the two commits because they still seem fairly well self-contained to me, and the first commit isn’t broken or anything without the second one.

Copy link
Member

@dracos dracos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks for all the changes :)

@dracos dracos merged commit 2b0e62e into master May 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants