[Footer]: Fix footer social icon hit area - flexible by joshbruce · Pull Request #1262 · uswds/uswds

joshbruce · 2016-06-16T17:04:41Z

Description

Alternative #1248
Fixes #346

Additional information

#1248 updates the size of the social link icons, and adds a hit area

around the links to make them more mobile friendly. Visually, the links
are smaller; however, the active area of the link is larger (~45px
square).

Things I noticed from creating #1248:

Changing icon sizes ([Footer] design update #706) requires manually changing the height and
width per link - in two areas (once for the svg element and the other
for the image element).
Adding a new icon (LinkedIn, for example), requires adding 6 new
lines to define the link.
The CSS definition assumes all social links need to be held within a
container .usa-social-links, which has no definition outside of
defining each a element within the container.

This PR:

Re-engineers social icons as background images.
Removes dependency on .usa-social-links container.
Removes containing div.
Adds .usa-social-link class for defining global attributes (hit
area, for example); allowing any link to become a social link.
Adds .facebook, .twitter, .youtube, and .rss classes, which
must be used in conjunction with .usa-social-link to take effect.
Allows for introduction of more background for social media links by
extenders, without having to define dimensions and layouts.

## Description Fixes #346 ## Additional information * This PR uses `display: inline-block` to allow setting height and width to an inline element. * Uses 44 pixels for both height and width per the Apple iOS HIG * Concerns with `text-align: right` - could result in thumb hitting to the right of the hit area - actually hitting the icon to the right of the actual target. `text-align: center` used instead. * Using `text-align: center` causes the right alignment of the icons to be off when compared “Agency Contact Center” text. `position: relative; left: 13px` used in order to compensate. Slight concern that this could cause a horizontal scroll bar to appear on mobile - though, given the overall alignment of the `social-links` container - concern may be moot.

- changed icon base sizes - made 2x for high-density displays - converted to rem - hit area larger than 44px

## Description Alternative #1248 ## Additional information #1248 updates the size of the social link icons, and adds a hit area around the links to make them more mobile friendly. Visually, the links are smaller; however, the active area of the link is larger (~45px square). Things I noticed from creating #1248: - Changing icon sizes (#706) requires manually changing the height and width per link - in two areas (once for the `svg` element and the other for the `image` element). - Adding a new icon (LinkedIn, for example), requires adding 6 new lines to define the link. - The CSS definition assumes all social links need to be held within a container `.usa-social-links`, which has no definition outside of defining each `a` element within the container. This PR: 1. Re-engineers social icons as background images. 2. Removes dependency on `.usa-social-links` container. 3. Removes containing `div`. 4. Adds `.usa-social-link` class for defining global attributes (hit area, for example); allowing any link to become a social link. 5. Adds `.facebook`, `.twitter`, `.youtube`, and `.rss` classes, which must be used in conjunction with `.usa-social-link` to take effect. 6. Allows for introduction of more background for social media links by extenders, without having to define dimensions and layouts. This is a branch from a branch (could not get the image updates from #1248 in here - possibly a knowledge gap on my part); therefore, if we would like to go this route, I should be able to merge this into the other to update #1248…at which point, this PR can be closed without merging.

joshbruce · 2016-06-16T17:06:20Z

@maya and @ericadeahl - What SVG compiler are you using - I see the SVG generated for this is huge compared to what it was.

joshbruce · 2016-06-16T17:08:56Z

shawnbot · 2016-06-17T16:27:59Z

I like this approach a lot. FWIW, I think that the inline SVGs could have been sized in CSS and the <image> elements given width="100%" height="100%" to solve the redundant dimensions issue. This approach is even cleaner, though, and requires a lot less markup for users to implement, which is always a good thing.

maya · 2016-06-17T16:39:20Z

+  margin-left: $blank-space;
+  position: relative;
+  text-align: center;
+  width: $hit-area;


I don't think we need variables. We only use variables if the value is repeated at least twice and the value is likely to be updated at least once. Can you remove?

I generally agree, but I think that the $hit-area variable is helpful here. @joshbruce maybe move the comments alongside the properties instead?

Hmmmm...either way is fine for me.

The initial reason - specifically for the hit area one is actually because, according to the CSS coding style, I couldn't put width and height next to each other alphabetically. The other two came about just to further separate the static values from the implementation - OOP-style.

Again, I'm fine for whichever.

I'm OK with keeping $hit-area for that reason, but would remove the rest.

Can we add a more descriptive comment on it:

Link hit target is 44 x 44 pixels following Apple iOS Human Interface Guidelines

Added a background-height variable - which could make future changes easier should we change the height and width of the links or changing the proportional height of the background icon.

rogeruiz · 2016-06-20T15:32:08Z

Hey @joshbruce. Looks like you'll need to rebase this branch onto the latest work on staging. #1261 has removed screener and that will get this PR to pass in CircleCI. If it's helpful, I can rebase these changes manually and submit a new pull request with that update. Thanks!

…n-hit-area-flexible

joshbruce · 2016-06-20T16:38:24Z

@rogeruiz - Thanks. Saw the traffic and wasn't sure. Will push in two seconds and see what happens.

Might need some follow-on help, if you're up to it.

maya · 2016-06-21T16:30:52Z

@joshbruce looking at the branch, this is what it looks like:

This is what it's supposed to look like:

@ericadeahl posted sizes for the icons in #1248 (comment) although they all appear to be the same 29px x 29px here.

maya · 2016-06-21T16:42:14Z

+  @extend .usa-social_link;
+  background-image: url("../img/social-icons/png/rss25.png");
+  background-image: url("../img/social-icons/svg/rss25.svg");
+}


Add one line-break here. All of our files should have an extra space at the end of the file.

@maya - I'm not sure why the blank line is not showing in the diff...? I added periods to the comments in order to get this commit to work. My file shows 350 line long.

Thoughts?

It must be a github thing, just checked this out locally and looks correct. 👍

joshbruce · 2016-06-21T16:53:10Z

@maya - Odd. Here's what I see:

The screenshot is from Safari 9.1.1 on the macOS 10.11.5.

The next one is from Chrome 51

Thoughts?

maya · 2016-06-21T16:56:48Z

@joshbruce was on the wrong branch, my bad! Looks like your screenshots :)

maya · 2016-06-21T17:11:53Z

@joshbruce on mobile, the alignment looks like it's indented to the right, could we align this flush left?

Should be:

joshbruce · 2016-06-21T19:14:38Z

@maya - Good catch - sorry about that.

Not quite an exact match - mainly because I don't think the "should be" image accounts for the margin and increased height and width. Having said that, the spacing is consistent with the medium screen.

Would you be willing to school me on how you managed to see the branch from this PR? I was trying to that with my program's standards the other day and could not get it to show up.

joshbruce · 2016-06-21T19:37:54Z

@maya - Sorry - forgot the spacing between the icons on top and bottom - should be good now.

maya · 2016-06-23T19:49:16Z

Looks good!

shawnbot · 2016-06-23T20:04:38Z

Woohoo, thanks @joshbruce!

Josh Bruce added 6 commits May 27, 2016 10:44

Merge remote-tracking branch '18F/staging' into staging

f38e9b1

Merge remote-tracking branch '18F/staging' into staging

da4beb1

Merge remote-tracking branch '18F/staging' into staging

deabb59

Updated based on feedback

ab9c2a1

- changed icon base sizes - made 2x for high-density displays - converted to rem - hit area larger than 44px

Revert SVG

2f3993f

joshbruce changed the title ~~Fix footer social icon hit area flexible~~ [Footer]: Fix footer social icon hit area - flexible Jun 16, 2016

joshbruce mentioned this pull request Jun 16, 2016

[Footer]: Fix footer social icon hit area #1248

Closed

maya reviewed Jun 17, 2016
View reviewed changes

juliasolorzano added [Type] Enhancement labels Jun 17, 2016

juliasolorzano assigned shawnbot and maya Jun 17, 2016

Josh Bruce added 2 commits June 17, 2016 18:47

Updated based on feedback

59d809f

Added a background-height variable - which could make future changes easier should we change the height and width of the links or changing the proportional height of the background icon.

Update code comments

2468ea3

joshbruce mentioned this pull request Jun 18, 2016

[Website, Label, Alerts, Links] Background color and text color mixin and classes #1242

Closed

6 tasks

Merge remote-tracking branch '18F/staging' into fix-footer-social-ico…

b32a88a

…n-hit-area-flexible

maya reviewed Jun 21, 2016
View reviewed changes

Faux commit

cd32540

Mobile alignment fixes

37621f0

Fixed line-heights

61a6908

maya merged commit 29aac95 into uswds:staging Jun 23, 2016

maya removed the [Status] In progress label Jun 23, 2016

donjo unassigned shawnbot Jan 22, 2018

Uh oh!

Conversation

joshbruce commented Jun 16, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Additional information

Uh oh!

joshbruce commented Jun 16, 2016

Uh oh!

joshbruce commented Jun 16, 2016

Uh oh!

shawnbot commented Jun 17, 2016

Uh oh!

maya Jun 17, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shawnbot Jun 17, 2016

Choose a reason for hiding this comment

Uh oh!

joshbruce Jun 17, 2016

Choose a reason for hiding this comment

Uh oh!

maya Jun 17, 2016

Choose a reason for hiding this comment

Uh oh!

rogeruiz commented Jun 20, 2016

Uh oh!

joshbruce commented Jun 20, 2016

Uh oh!

maya commented Jun 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maya Jun 21, 2016

Choose a reason for hiding this comment

Uh oh!

joshbruce Jun 21, 2016

Choose a reason for hiding this comment

Uh oh!

maya Jun 23, 2016

Choose a reason for hiding this comment

Uh oh!

joshbruce commented Jun 21, 2016

Uh oh!

maya commented Jun 21, 2016

Uh oh!

maya commented Jun 21, 2016

Uh oh!

joshbruce commented Jun 21, 2016

Uh oh!

joshbruce commented Jun 21, 2016

Uh oh!

maya commented Jun 23, 2016

Uh oh!

shawnbot commented Jun 23, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

joshbruce commented Jun 16, 2016 •

edited

Loading

maya Jun 17, 2016 •

edited

Loading

maya commented Jun 21, 2016 •

edited

Loading