Social buttons need tweaks

[corysimmons]

https://amp-partners.appspot.com/social demo looks good but the Twitter logo isn't retina friendly (it isn't svg whereas the others are), and it doesn't look like Facebook is displaying properly at all.

Also is there a reason these have border-radius? It's not built in is it? Perhaps we shouldn't encourage additional styles (to disable pre-existing styles) where possible?

[jaygray0919]

The demo displays in Chrome and Safari as shown be @corysimmons. In Firefox, however, only the two Twitter images display. The four images to the right of "Moo!" are color boxes. Anyone else see this condition? Is there a Firefox setting that, if turned off, would show only the color box and not the icons?

[dvoytenko]

/cc @paul-matthews could you please take a look?

[rudygalfi]

I believe @paul-matthews will be unavailable for a couple weeks.

[ericlindley-g]

@dvoytenko , who's a good person to take a look at this while Paul is unavailable?

[dvoytenko]

@ericlindley-g I think these are all done. Can we confirm this?

[rudygalfi]

Rolling this into next milestone.

[ericlindley-g]

@mkhatib , could you check locally to make sure the issues on this page are fixed? (Twitter icons should be SVG; Facebook icons should show up, rather than displaying "Moo!")

[mkhatib]

@ericlindley-g I confirmed that twitter icons are SVG. For FB this is actually working how it was designed to work. When the user provides their own anchor element it's assumed that they want to style it themselves hence our styles are not applied.

This is somewhat confusing and I am working on a way to make this less confusing. But in the example provided if you remove the custom DOM elements the FB icon and styling would show because our defaults kicks in.

I am working on a way to streamline this somehow.

[ericlindley-g]

Thanks for checking—I looked at the example more closely after your response, and I see that it's working as intended.

on Firefox, though, I'm seeing the issue that @jaygray0919 reported (only colored squares, without logo content, after the "moo!"s). Would that be something you could take a look at, too? Happy to open another issue to track, if that helps.

[mkhatib]

Ah sorry missed that part. You're correct this does seem to happen on firefox. From MDN:

Note: as of Firefox 6, fragments (anchors) are processed consistent with other URI schemes, thus a bare "#" in the content needs to be escaped as '%23'.

I confirmed that replacing all # in the fill= attribute with the escaped %23 seems to fix the problem.

No action is required though, it seems this has been fixed in #2665. The example link given uses its own build of AMP and not our production or canary one hence you won't be able to see latest changes to that example page. I created this JSBIN where you can preview the latest changes - you still need to toggle the experiment by running AMP.toggleExperiment('amp-social-share') in the console in that page.

[ericlindley-g]

Looks good! @mkhatib, you mentioned above that you're working on making the behavior a little less confusing, but it seems like the issues here are largely fixed or working as intended. Would you like to leave the issue open for any additional work you're doing, or can we close it?

[mkhatib]

I'd keep this as the general tracking bug for the changes - but I am ok with closing it as well.

[ericlindley-g]

Rolling into the next milestone

[mkhatib]

Closing this, please open separate issues if anything is still not addressed by the last PR.