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

Replace ical and office calendar image icons #17282

Merged
merged 5 commits into from
May 22, 2020

Conversation

agh1
Copy link
Contributor

@agh1 agh1 commented May 10, 2020

Overview

This slices off one commit from #17259 for ease of review. I then added an improved way to display the icons and applied the same improvement in a parallel place.

This changes the icons on the bottom of the event info page to use Font Awesome. These are stacked icons, the left one showing a calendar with a download icon (to download a file for the one event), and the right one showing a calendar with a link (to subscribe to the calendar).

The original icon files were used nowhere else, so this removes them.

I also altered the Event Dashboard to use similar icons for similar things at the top. The four links (view a listing, subscribe to a RSS feed, download a calendar, link to a calendar feed) have always been difficult to differentiate with icons, so this should be helpful.

Before

image

image

After

image

image

Technical Details

Stacked Font Awesome icons don't always line up with non-stacked ones, so it's helpful that all of the icons together be stacked.

@civibot
Copy link

civibot bot commented May 10, 2020

(Standard links)

@jaapjansma
Copy link
Contributor

  • General standards
    • Explain (r-explain)
      • PASS : The goal/problem/solution have been adequately explained in the PR.
    • User impact (r-user)
      • PASS: The change would be intuitive or unnoticeable for a majority of users who work with this feature.
    • Documentation (r-doc)
      • ISSUE: The developer documentation should be updated.
      • COMMENTS: We think it might be nice to also document the changes in this PR in the developer documentation.
    • Run it (r-run)
      • PASS: We checked in both dmaster and the test environment of this PR. We have checked the Dashboard of Events, the manage event screen and the public page of an event
      • ISSUE: We do think it might be worth to also replace the icons on the manage event screen
  • Developer standards
    • Technical impact (r-tech)
      • PASS: The change preserves compatibility with existing callers/code/downstream.
    • Code quality (r-code)
      • PASS: The functionality, purpose, and style of the code seems clear+sensible.
    • Maintainability (r-maint)
      • PASS: The change sufficiently improves test coverage, or the change is trivial enough that it does not require tests.
    • Test results (r-test)
      • PASS: The test results are all-clear.

@agh could you also implement the icons on the manage event page?

@mattwire mattwire added the needs-work-not-review-ready Submitter required to take action, will be closed after 1-2 weeks in this state label May 11, 2020
@agh1 agh1 force-pushed the calendar-icons branch from 74f3c6d to 4765c38 Compare May 11, 2020 21:11
@agh1 agh1 force-pushed the calendar-icons branch from 4765c38 to 679bd09 Compare May 11, 2020 22:47
@agh1
Copy link
Contributor Author

agh1 commented May 11, 2020

@jaapjansma @mattwire I updated this to do the same thing on Manage Events as on the CiviEvent dashboard. Since it's a lot of repeated code, and it's weird that the URLs were all defined in Smarty, I created a new CRM_Event_BAO_Event::getICalLinks() function that supplies them.

@agh1
Copy link
Contributor Author

agh1 commented May 13, 2020

I updated this with aria-hidden per #17318

@eileenmcnaughton eileenmcnaughton removed the needs-work-not-review-ready Submitter required to take action, will be closed after 1-2 weeks in this state label May 13, 2020
@eileenmcnaughton
Copy link
Contributor

test this please

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

Successfully merging this pull request may close these issues.

5 participants