Update success.tpl CSS (pre/post upgrade messages)

[mlutfy]

Overview

Initially I just wanted to add a link to the "CiviCRM System Status", because post-upgrade, I think that's the first thing people should check.

However, after adding the link, it was ugly, just felt like I was adding more clutter, so I went down a rabbit hole attempt to cleanup a bit.

• Pre-upgrade: Hides the "cancel" button, because it's clutter and people know how to use a back button
• Bigger font-size, sans-serif
• Less bold, because it's annoying; replaced by even bigger text, when relevant
• Align icons (mostly on the post-upgrade messages)

Before

Before running the upgrade:

After running the upgrade:

After

Before running the upgrade:

Before running the upgrade (if there are no upgrade warnings):

After running the upgrade:

Comments

Some of the funnyness around buttons is because of Shoreditch and can be ignored.

I think the post-upgrade message should be simplified with a more clear call-to-action (donate), but it would break translations and I was not in the mood.

[civibot]

(Standard links)

• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[eileenmcnaughton]

@vingle @artfulrobot any input?

[demeritcowboy]

Other than the mysterious "X" which displays an undefined variable error instead of the preupgrade message, this looks nice. +1

[demeritcowboy]

One more note: If this is merged, the hardcoded color: white should be removed from General.php since now if you previously visited that <a> link, which you may well have, it appears white on light yellow, and is almost invisible.

[vingle]

+1 to cleaning up this screen, and the new design looks much nicer.

However the PR's markup continues many of Civi's legacy/headache patterns:

• writes css inline, rather than using Civi's utility alert message type classes like .help or .messages. Above anything, that makes it a headache to theme.
• uses em units rather than the more accessible/current rem units. Ems are relative only to the parent container so aren't consistent (an h1 sized 3em could be a different size depending where it is on the page).
• does 'pixel pushing' to position elements, which isn't so durable as sizes change over time. Like the inline css, someone would need to rewrite that someday.
• uses sprite images, ie <span class="crm-status-icon success"></span> - which I think we're trying to phase out for Font Awesome.

Is it possible to:

• remove the inline CSS and instead use .crm-container .status for the yellow background warning message and .crm-container .help for the green bg success message?
• rather than size the larger font inline, use something like .crm-container h3.nobackground.
• swap the image sprites for Font Awesome, ie .fa-check and .fa-exclamation-triangle.

[mattwire]

@mlutfy Some good feedback from @nicol and @demeritcowboy. If you can address those things I'll merge it quickly before we start swapping wheels.

[mlutfy]

Anyone know why this page displays with the frontend theme in Drupal?

The frontend themes usually don't support many of the backend themes (ex: alerts, maybe font-awesome).

[vingle]

@mlutfy – ah I see. It does load on WordPress and Joomla. Font Awesome loads on my Drupal 9 upgrade screen tho, so that should be safe.

As you're adding that CSS just to support Drupal, what about instead adding a class that Drupal should recognise like messages messages--warning and messages messages--status to get the Drupal alert style? ie

<div class="crm-container">
 <div class="messages messages--warning status">
  This messages will have either Drupal's yellow warning style or Civi's
  </div>
</div>
<div class="crm-container">
 <div class="messages messages--status help">
  This messages will have either Drupal's green alert style or Civi's
  </div>
</div>

[mlutfy]

@vingle Thanks for the suggestions. I tried to implement some of the suggestions, to the extent that this is a frontend form and I think we should render as a backend form before doing more changes, but I went way too far down the rabbit hole. I'd rather not add Drupal-specific classes, I just wanted to avoid random visual results depending on the CMS. I removed as much CSS as I could, and hopefully we can remove most of it when we flip to a backend form.

@demeritcowboy I removed the white link from General.php, but it's a b it difficult to test. Since I was breaking the translations for this string, I fixed the ts usage.

I also decided to break/fix that "thank you" string, because it was really annoying me.

Here is what it looks like on Drupal9 with the new changes (I replaced em with rem, but I'm not too familiar with those differences, so I improvised):

I don't know the logo will be missed, but I figure that simpler is better, and I still think that message should be simplified, and keeping that out of scope for now:

This is after fixing the "Thank you" message:

While grave-digging, found some possibly un-used things, but wasn't sure:

• templates/CRM/common/upgradeCleanup.tpl
• in css/admin.css there is .upgrade-success that is only used by upgradeCleanup - that CSS file is mostly rubbish and I'd rather avoid it, even if the upgrade screen uses it

[demeritcowboy]

Something's wrong with the donation link. I think it's because it's now in single quotes - checking...

re: frontend theme: https://github.com/civicrm/civicrm-drupal/blob/0ec6bf7886d03205b0f4f74533357525efecb552/modules/civicrmtheme/civicrmtheme.module#L91

[demeritcowboy]

Yeah it's the single quotes. You can either do

3="href=\"https://civicrm.org/contribute?src=ug&sid=$sid\" target=\"_blank\""
or
3="href='https://civicrm.org/contribute?src=ug&sid=$sid' target='_blank'"

I think the first produces more "standard" ('standard'?) html, but the second looks nicer in the code.

And yes the General.php link looks better now. The message itself is still worded incorrectly most of the time, but that's a separate issue.

[vingle]

re: frontend theme: https://github.com/civicrm/civicrm-drupal/blob/0ec6bf7886d03205b0f4f74533357525efecb552/modules/civicrmtheme/civicrmtheme.module#L91

Is that a way to adjust this? Maybe it would be better to deal with the quirk of Drupal viewing a backend form as a front-end form, rather than making every CMS load styles that clash with any non-Grenwich Civi theme that's running? The hard-baked styles will also clash with whatever comes after Greenwich…

[demeritcowboy]

My guess is that it was there because during an upgrade you want as little as possible code running to avoid things like chicken-and-egg situations, but if other cmses have already been running for a long time with the civi theme during upgrade then it doesn't seem a big risk. And it seems drupal 9 doesn't have that if clause.

[mlutfy]

jenkins, test this please

[mlutfy]

@demeritcowboy Thanks! - fixed the links, I think?

There are weird test fails:

[[Download build-1 (type 'drupal-clean' in '/home/jenkins/bknix-dfl/build/build-1')]]
[[Update caches]]
SKIP: git_cache_setup 'https://github.com/civicrm/civivolunteer.git' /home/jenkins/bknix-dfl/build/.civibuild/cache/civicrm/civivolunteer.git' (recently updated; ttl=60)
SKIP: git_cache_setup 'https://lab.civicrm.org/extensions/angularprofiles.git' /home/jenkins/bknix-dfl/build/.civibuild/cache/ginkgostreet/org.civicrm.angularprofiles.git' (recently updated; ttl=60)
error: could not lock config file config: File exists
fatal: could not set 'remote.origin.url' to 'https://lab.civicrm.org/extensions/civirules.git'

(gitlab seems to be working fine, and I checked fail2ban/firewall)

And thanks for the pointer in civicrmtheme. I found this commit:
civicrm/civicrm-svn@e5cd2cb

which leads to: https://issues.civicrm.org/jira/browse/CRM-10612 - but not very detailed. It was on Drupal7, 4.1 to 4.2 upgrade.

I was testing on Drupal9 though. And as you say, on WordPress this does not apply, and CiviCRM are less brutal than before, so I would be curious to try removing that code, but separate PR/repo.

[mlutfy]

jenkins, test this please

[mlutfy]

For what it's worth, I tested WP too:

[demeritcowboy]

Have put merge-ready.

I don't know why I didn't see it but drupal 9 does also have the check for upgrade so returns early. But can treat as a followup if desired.

[mlutfy]

Thanks! I opened PRs on the Drupal repos to change that behaviour:

• Use the backend theme when upgrading civicrm-drupal-8#81
• Use the backend theme when upgrading civicrm-drupal#665

[demeritcowboy]

@mlutfy What order did you want to do this in? I'm ok to merge the other PRs, in which case would you then update this or should we also merge this and leave further updates for now?

[eileenmcnaughton]

Would it be possible to add a comment to success.tpl explaining why putting css in the template is acceptable in this situation?

[mlutfy]

@eileenmcnaughton Yep, comment added.

@demeritcowboy PRs Can be merged independently. I'll circle back at some point to cleanup the CSS. (the Drupal changes seem harmless to me, but who knows, maybe it'll need to be reverted).

[demeritcowboy]

I'll merge the other ones. I'm not sure what you just changed here but something is off now (drupal 7). It really wants to warn me about something:

[mlutfy]

Is that the new Montreal theme? That looks like a typical road in downtown Montreal during construction season (i.e. summer).

I'll circle back and get rid of the CSS and do more testing :)

(I had only added the CSS comment, but maybe the backend theme is causing issues?)

[mlutfy]

I did some minor updates and more testing on all CMSes:

• Replaces the old CiviCRM status/checkbox icons with font-awesome (as suggested by @nicol)
• Avoid using generic success or warning classes because they conflict with the Drupal theme (avoid Montreal-construction-style visual)

I could not find suitable generic CSS classes from CiviCRM to avoid inline CSS. There are classes for alerts (that appear in the corner of the screen), but they do not apply to full-screen messages. I updated the comment to that effect.

Drupal7 with Seven and Shoreditch, pre/post upgrade:

WordPress pre/post-upgrade:

Without Shoreditch:

Drupal9:

[vingle]

This is looking great. Thanks for adding FA.

I could not find suitable generic CSS classes from CiviCRM to avoid inline CSS. There are classes for alerts (that appear in the corner of the screen), but they do not apply to full-screen messages. I updated the comment to that effect.

.crm-container .status and .crm-container .success I think are the full width yellow and green regions this page has been trying to copy. Had a quick go in WordPress/Greenwich in case you want to ditch the CSS from the template…

<div class="crm-container">
    <div class="status">
      <form method="post">

and

<div class="crm-container">
    <div class="help">
      <p>…</p>
       <div>
          <h2>&nbsp;<i class="crm-i fa-check" aria-hidden="true"></i> Congratulations message…</h2>
        </div>

gives (with the .tpl css commented out):

and

[mlutfy]

@vingle I'm a bit reluctant to tweak further, because I feel like the generic core classes are insufficient, and they should always be prefixed to avoid a conflict with the CMS theme (for example, "labels", but also .success/.warning/.status can be really random).

Similarly, using class="help" and class="status" as wrappers seems like we're using those classes because it works, but not because we have any documented UI elements/patterns.

I know this topic comes up often, but is there some sort of documentation on that? If not, can we create something from scratch using : https://github.com/civicrm/org.civicrm.styleguide ? (which was probably never adopted because shoreditch-centric)

[vingle]

@mlufty fair enough - good reasons. Tho it's a pattern used pretty widely across Civi, so at the point at which we are able to replace all <div class="status"> with <div class="crm-alert-warning"> matching the bad pattern here will make it easier to find later ;).

The current focus for myself and @artfulrobot for a replacement to the Style Guide is Theme Test. I've added an issue to add some snippets for current Alert handling.

[demeritcowboy]

I gave it a last little run and it sounds like it's more or less agreed to leave more tweaking until later.

I notice the donation link still has src=ug which is left over from before when it was a different link, but it doesn't hurt anything. It could still be useful to have even if not being tracked.