voteamerica / voteamerica.github.io

Front end for the Carpool Vote website. Support every American's right to vote!
http://www.carpoolaction.org/
MIT License
38 stars 50 forks source link

Replace Facebook & Twitter widgets with plain styled links #338

Closed mikeesto closed 6 years ago

mikeesto commented 6 years ago

Resolves #337

Buttons are definitely loading faster, begone slow JS :+1:

richardwestenra commented 6 years ago

This looks great! Thanks @MichaelE1 :)

I'll test it when I'm able. Overall the code looks very good, and should be fine to merge once I've tested it. In the meantime, I've got two minor suggestions if you feel like actioning them, but no worries if not:

  1. The two image files could be smaller. They'll be shown at 16px x 12px, and you'll want them double that size for retina screens, but there's no need for them to be larger than that. I'd resize them to 32px x 24px (or 32px x 32px if they're square?). Also have those images been optimised for web? I haven't checked but please run them through ImageOptim if you can. No big deal though.
  2. There's a fair bit of duplicate CSS props across the different social buttons. You could abstract a lot of it to shared rules, e.g. something like this:
    
    .social a {
    display: inline-block;
    color: white;
    text-decoration: none;
    border-radius: 3px;
    font-size: 0.6875em;
    font-weight: bold;
    padding: 5px 6px 3px;
    font-family: Helvetica, Arial, sans-serif;
    vertical-align: top;
    }

.social a:before { content: ''; background-size: contain; width: 16px; height: 12px; display: inline-block; margin: -1px 4px 0 0; vertical-align: top; transform: scale(0.9); }

.social__twitter a { background-color: #00aced; } ....


Then just use the more specific classes for each button for just the specific background colours and images etc. Keeps things DRYer, hence more maintainable and smaller file-size. What do you reckon?

Thanks again!
mikeesto commented 6 years ago

Thanks for the feedback @richardwestenra . I'll address both your suggestions so hold off merging for now :)

mikeesto commented 6 years ago

OK it should be good for testing now @richardwestenra . Abstracted the CSS and reduced the sizes of the icons. Thanks.