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

2005 called and they wanted their icons back #17259

Closed
wants to merge 10 commits into from

Conversation

agh1
Copy link
Contributor

@agh1 agh1 commented May 7, 2020

Overview

So @demeritcowboy opened up a can of worms in #17245 (comment) when he mentioned that the Font Awesome chevrons in the admin console made it look as if each item was a section to open.

Of course, in the admin console, each section is a whole section to open, and they're wild:
image

Getting rid of those has been a bugaboo of mine ever since the Maryland sprint a number of years ago, and I decided to go for it. It isn't as colorful and it loses its historic flavor, but it doesn't bury the descriptions under a tiny triangle:
image

The thought is that the only reason someone ends up at the admin console nowadays is that they're somewhat lost and want an explanation. (Or Javascript isn't working and the menu's gone.)

The new admin console is also responsive, going to a single column for smaller screens.

But wait, there's more!

I knew that most of the icons in there were exclusively for that page, so I decided to grep for the names of everything in the i folder to see what could be deleted. In the process, I discovered a few blast-from-the-past icons that still pop up a lot.

check.gif

This green checkbox would show up in a bunch of places, especially in tables of items where it denoted the default item. The logic repeated itself a lot, so I created a new function, CRM_Core_Page::getDefaultCheckMarkup(), that gets a checkmark along with text saying "Default". There's also a Smarty function, {defaultCheck}, that does the same and even allows for passing the value that determines whether the checkmark appears. I extended it to support other text like "Counted", and you can also supply other icon names.

image

copy.png

The icon to copy items down the column on the batch edit screen was another one borrowed from Geocities. Similarly, I found that the code was basically identical from page to page, so I wrote a Smarty function called {copyIcon} that provides the icon and explanatory text in a single place.

image

geotag_16.png

This was a fairly straightforward swap of the old map pin icon for the Font Awesome one.

image

office-calendar.png and ical_feed.gif

The solution for these isn't great because there's only so many ways to denote a calendar, but the old one were particularly hideous. The left-hand one downloads an ical file for the event, while the right-hand one subscribes to an ical feed that offers the one event.

image

stop-icon.png

I have a lot of warm feelings for this little pixelated octagon which very faintly says STOP in English. It had to go, though. Font Awesome (even the newer one) also lacks a stop or octagon icon.

Instead, I figured it might make as much or more sense to put a universal no sign on top of an icon representing the thing you shouldn't do: a phone, a mobile phone (for SMS), an envelope, or a paper airplane (for email).

image

There's a triangle with an exclamation point for email addresses that are on hold.
image

This also gets a smarty function, {privacyFlag} to standardize the markup and simplify adding it.

Semantic vs. decorative icons

One advantage that traditional image icons have over font icons is alt text. In the first pass on updating icons a few years ago, we were mostly replacing sprite-based icons which didn't have alt text. In contrast, the icons being replaced here convey distinct meaning. In many cases, the alt text would describe much more than the icon itself.

Title attributes help to some degree, but screen readers don't necessarily pick them up. We need a way to add text that screen readers will see but that won't clutter the visual appearance. There's a commit in this PR that I opened as a separate PR #17255 that adds a new sr-only class for text that will only be read by a screen reader. The changes in this PR use that class heavily.

@civibot
Copy link

civibot bot commented May 7, 2020

(Standard links)

@demeritcowboy
Copy link
Contributor

I don't think I ever realized the triangle to the left of each section header was actually clickable. I like the toaster that makes toasty coins under civicontribute.

@colemanw
Copy link
Member

colemanw commented May 7, 2020

@agh1 I really like the cleaner look of the new admin screen!
It might be possible to use fa icons in each of the headers (many of them have corresponding icons in the menubar) but I know that gets tedious to pick icons for ambiguous things like "Profiles".

@eileenmcnaughton
Copy link
Contributor

I love the issue title!

@agh1
Copy link
Contributor Author

agh1 commented May 8, 2020

Per @colemanw's suggestion I reworked the {defaultCheck} into a new icon function that is more general and is better for translation. The usage is

{icon icon="fa-space-shuttle" condition=$is_launched}{ts}Out of this world{/ts}{/icon}

I pushed the conditional logic down to the PHP function, which is now more generic too. Both now also handle the situation of an icon having no text. That means an icon can be dropped into a smarty template with simply

{icon icon="fa-anchor"}{/icon}`

or

$taxiMarkup = CRM_Utils_Page::crmIcon('fa-cab');

Separately, in going around and fixing things, I realized that in some spots the BAO or page class was sending the full icon markup to the template rather than letting the template render the icon. Three of these examples were in datatables, so I wrote a new CRM.utils.formatConditionalIcon() to provide this in Javascript.

@agh1 agh1 force-pushed the admin-console branch 2 times, most recently from 1d27ce7 to 1b52184 Compare May 8, 2020 20:41
@agh1
Copy link
Contributor Author

agh1 commented May 9, 2020

I promise I'm not trying to make this enormous PR even bigger, but in doing other things I noticed that CRM_Admin_Page_Admin was doing all kinds of stuff that isn't needed in the updated admin console, and in all its hard-coding, it forgot CiviCampaign.

@mattwire
Copy link
Contributor

@agh1 Needs rebase. Also, is there anything obvious that can be broken out into smaller PRs for ease of review?

@agh1
Copy link
Contributor Author

agh1 commented May 10, 2020

@mattwire I've opened seven PRs that together make up the changes here.

@agh1 agh1 closed this May 10, 2020
@eileenmcnaughton
Copy link
Contributor

I sent the following message to the dev list -

Hi all,

I just wanted to draw people's attention to the above PR #17259

(that PR has been closed but many subparts have been merged)

It simplifies the admin screen - which may alter the appearance in various themes so you might want to check how it looks in shoreditch / any other theme you use).

Eileen

@artfulrobot
Copy link
Contributor

artfulrobot commented May 15, 2020

Just found this thanks to @eileenmcnaughton 's email. Great work! Me an Nicol were bemoaning how difficult theming Civi is because of the veritable museum of how-to-do-icons that it is. I did various hacks in my Aah theme to get rid of the icons I couldn't abide (don't think I've done anything worth sharing there, though, and not nearly as comprehensive as this). These PRs seem a good step forward.

What Civi version are we targetting for release 5.27?

@agh1
Copy link
Contributor Author

agh1 commented May 15, 2020

For cross-reference purposes, here's an updated list of related pull requests from the past week or two.

Utility functions and structural changes

Straightforward replacements:

Most have been merged; the remaining open ones are #17282, #17296, #17297 (which needs #17296 merged first), #17318, and #17319.

@agileware-justin
Copy link
Contributor

Great work @agh1 and other collaborators :)

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