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

changes social media iframes/scripts to links, simplifies markup, adds email & bootstrap classes #18880

Merged
merged 3 commits into from
Nov 22, 2020

Conversation

vingle
Copy link
Contributor

@vingle vingle commented Oct 30, 2020

Overview

As discussed here, updates the social media sharing footer:

  • replaces embedded scripts with direct sharing urls
  • adds an email share button
  • adds Bootstrap classes
  • uses <button> for visual fallback in absence of Bootstrap or Greenwich
  • adds IDs to buttons to let each be targetted with unique css
  • designed and tested against three scenarios: consistent Civi UI with Greenwich, Bootstrap (from CMS or a Civi theme) & without any Civi theming

Before

With Greenwich/automatic set in CiviCRM Display Preferences:

image

Without Greenwich:

image

After

With Greenwich/Automatic set

image

Without Greenwich, but with a Bootstrap-based CMS theme:

image

Without Greenwich, without a Bootstrap-based theme:

image

Technical Details

This footer appears at the bottom of contribution and event pages and also is included in a few email tempaltes.

The markup for use in email templates is enabled under {if $emailMode eq true} and uses <a> with Bootstrap css classes (not Civi button classes).

For the contribution/event page footers, <button> is used with an onclick behaviour, in order to provide browser-default button appearance independent of civicrm.css or Bootstrap. IDs are added to each button to support targetted CSS (ie different colours).

Comments

The use of this in email templates hasn't been tested.

@civibot
Copy link

civibot bot commented Oct 30, 2020

(Standard links)

@civibot civibot bot added the master label Oct 30, 2020
@kcristiano
Copy link
Member

@vingle Thanks for undertaking this. I've r-run this on WordPress, Drupal 7 and Drupal 8. The changes are apparent and an improvement on all. I don't have an easy way to test on Joomla, but I imagine you've done that.

I see this as a good improvement and well discussed in the issue, so I'd like to see this merged.

@vingle
Copy link
Contributor Author

vingle commented Nov 16, 2020

Thanks for the review @kcristiano. Yes my tests were on Joomla. I've just setup Backdrop to test and it works as expected there too.

@homotechsual
Copy link
Contributor

This LGTM - confirming results of the R-Run from others. @eileenmcnaughton or @seamuslee001 can we get this merged? It's a good change and it has some privacy-positive implications.

@seamuslee001
Copy link
Contributor

merging as per review and there doesn't seem to be any objection on the lab ticket either

@seamuslee001 seamuslee001 merged commit 5e3aaf6 into civicrm:master Nov 22, 2020
@eileenmcnaughton
Copy link
Contributor

Yay

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants