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

CRM-18792, dev/core#378 - Create CSS theming subsystem #12929

Merged
merged 1 commit into from Jun 15, 2019

Conversation

@totten
Copy link
Member

commented Oct 12, 2018

Overview

This PR formalizes the "theming" construct. Key links:

This is an updated version of #8523 which addresses conflicts, emits a theme-selection hook, and displays the new settings in the admin UI.

Before

(Nothing to show)

After

On Drupal and Backdrop, the administration screen "Display Preferences" presents an option "Theme":

screen shot 2018-10-12 at 2 17 39 am

On WordPress and Joomla, the administration screen "Display Preferences" presents two options for the "Backend Theme" and "Frontend Theme":

screen shot 2018-10-12 at 2 16 45 am

Technical Details

Key programmatic interfaces:

  • theme_backend and theme_frontend: These settings can be manipulated via GUI, API, or civicrm.settings.php to specify one's theme preferences.
  • hook_civicrm_themes(array &$themes): This hook registers new themes. Generally, to create a new theme, one would make an extension, implement the hook, and drop-in some CSS files (css/civicrm.css and css/bootstrap.css). However, as discussed in the draft documentation, it supports other structures.
  • hook_civicrm_activeTheme(string &$theme, array $context): This hook determines the active theme on the current page-request. This is useful if you don't like the default classification of "frontend" vs "backend" pages.
  • Civi::service('themes'): The manager/repository for looking up metadata about the themes. It maintains the cache (list of available themes).

Comments

Functionally, this should be a superset of #12872: to prevent loading of Civi's CSS on frontend pages, set theme_frontend to none.

The draft documentation could use some more work, but hopefully it gets the key things across.

@civibot

This comment has been minimized.

Copy link

commented Oct 12, 2018

(Standard links)

@civibot civibot bot added the master label Oct 12, 2018

@totten totten removed the master label Oct 12, 2018

@eileenmcnaughton

This comment has been minimized.

Copy link
Contributor

commented Oct 12, 2018

test this please

@eileenmcnaughton

This comment has been minimized.

Copy link
Contributor

commented Oct 12, 2018

@totten let's squash to one - the commit chain is not that logical

),
'default' => 'default',
'add' => '4.7',
'title' => 'Backend Theme',

This comment has been minimized.

Copy link
@eileenmcnaughton

eileenmcnaughton Oct 12, 2018

Contributor

Matt has been adding ts() around these in the settings file, which makes sense

This comment has been minimized.

Copy link
@totten

totten Oct 15, 2018

Author Member

Updated

@@ -209,6 +209,23 @@
<td>&nbsp;</td>
<td class="description">{ts}Sort name format for individual contact display names.{/ts}</td>
</tr>

{if $config->userSystem->is_drupal EQ '1'}

This comment has been minimized.

Copy link
@eileenmcnaughton

eileenmcnaughton Oct 12, 2018

Contributor

so this implies Drupal has no backend theme which might be true from a drupal head POV but not so much from a ' I have front end & back end pages POV' - I don't think we need to solve that right now but some comments maybe.....

This comment has been minimized.

Copy link
@totten

totten Oct 15, 2018

Author Member

Right, there's the conceptual notion of frontend/backend (which can apply on any technology), and then there's the technical notion (which has been clearly defined in WP/J but not as clearly on D/B).

I put the conditionals in there because civicrm-core+civicrm-drupal haven't formally had the distinction, and it seemed like it'd be confusing to display both theme_frontend and theme_backend to a Drupal admin (when theme_frontend would be non-functional).

Of course, less officially, there's civicrmtheme.module which does create this concept on Drupal. So it might make sense to align with that. Seamus has another comment+PR getting at this same point... will try to put some of the reply in there.

This comment has been minimized.

Copy link
@eileenmcnaughton

eileenmcnaughton Oct 15, 2018

Contributor

@totten right - I was just thinking to get some code comments in there

This comment has been minimized.

Copy link
@JoeMurray

JoeMurray Oct 29, 2018

Contributor

I agree that just a bit more in the documentation about how existing approaches to admin themes in Drupal could migrate or take advantage of this would be nice (but not strictly necessary at this point).

@eileenmcnaughton

This comment has been minimized.

Copy link
Contributor

commented Oct 12, 2018

@totten given the amount of discussion that has already taken place (on the previous PR & here at the sprint) I'm feeling pretty good about this. It has unit tests so the hook signatures are looked in & where different approaches have been suggested I don't think anything has been clearly better so this seems like a great step forwards.

If we merge now we have 6 weeks before it goes live so we can iterate if need be - but of course most iterations will come later because we would only need a 'quick' iteration if we wanted to change the hook signature or contract in another way.

The minor changes I requested are

  1. add ts around title & description in the settings metadata
  2. comment the use of is_drupal for not offering 'front end' for drupal at this stage
  3. squash to one commit - I don't think the commits are a) applying cleanly for jenkins & b) well split up (there are wips & reverts in the mix) so just one seems to make sense now to me
if ($this->activeThemeKey === NULL) {
// Ambivalent: is it better to use $config->userFrameworkFrontend or $template->get('urlIsPublic')?
$config = \CRM_Core_Config::singleton();
$settingKey = $config->userFrameworkFrontend ? 'theme_frontend' : 'theme_backend';

This comment has been minimized.

Copy link
@seamuslee001

seamuslee001 Oct 14, 2018

Contributor

@totten @eileenmcnaughton one of the issues i raised in the PR from Agileware was that userFrameworkFrontend isn't set in Drupal integration (and likely not in backdrop either) but in Wordpress and Joomla

This comment has been minimized.

Copy link
@eileenmcnaughton

eileenmcnaughton Oct 15, 2018

Contributor

@seamuslee001 the way the form works is that the 'theme_frontend' setting is not exposed for drupal users (since it's not used) - I think that's fine at this stage. I think we could discuss further if drupal needs are met but I would prefer that as a follow up

@eileenmcnaughton

This comment has been minimized.

Copy link
Contributor

commented Oct 17, 2018

@totten can you rebase (& squash) this so tests will run & we can get it tidied away

@eileenmcnaughton

This comment has been minimized.

Copy link
Contributor

commented Oct 26, 2018

@totten my understanding is that this has been superceded & you have a functional extension (org.civicrm.themex) that can used as a dependency for anyone wanting to do theming & this https://github.com/totten/civicrm-dev-docs/blob/master-theme/docs/framework/theme.md is the current WIP documentation....

Should we close this? I would like to get the piece of work you started to a usable point if not perfect (remember that enemy of good :-)

@laryn laryn referenced this pull request Dec 4, 2018
@eileenmcnaughton

This comment has been minimized.

Copy link
Contributor

commented Feb 16, 2019

test this please

@eileenmcnaughton eileenmcnaughton force-pushed the totten:master-theming-rb branch from 6144872 to 24271bd Feb 16, 2019

@eileenmcnaughton

This comment has been minimized.

Copy link
Contributor

commented Feb 16, 2019

@totten I've rebased this so it's no longer stale but my understanding is perhaps it's superceded by https://github.com/totten/civicrm-dev-docs/blob/master-theme/docs/framework/theme.md and we are thinking to close it - I think we should resolve this one & also resolve making sure the docs point people in the right direction

@eileenmcnaughton eileenmcnaughton added this to requires more work to be reviewable in Product maintenance Feb 24, 2019

@eileenmcnaughton eileenmcnaughton added this to needs work to be reviewable in review board Feb 25, 2019

@eileenmcnaughton eileenmcnaughton moved this from needs work to be reviewable to needs agreement on tech approach in review board Feb 25, 2019

vingle referenced this pull request in mattwire/civicrm-haystacktheme Mar 3, 2019

@vingle

This comment has been minimized.

Copy link
Contributor

commented Mar 13, 2019

Is this PR going anywhere? The proposal seems a really good one: to let the users select front-end and back-end themes. So you could chose a Bootstrap or UIKit front-end theme developed to work for all CMSs and a Wordpress or Joomla-specific backend theme. And the interface proposal seems very intuitive.

@eileenmcnaughton

This comment has been minimized.

Copy link
Contributor

commented Mar 13, 2019

I worry this fell into the perfect being the enemy of the good trap ... @totten ...

@vingle

This comment has been minimized.

Copy link
Contributor

commented Mar 14, 2019

If there's anything I can do to help revive or encourage it's revival, please let me know!

@eileenmcnaughton eileenmcnaughton force-pushed the totten:master-theming-rb branch 4 times, most recently from 08d07ad to ebb1e2c Jun 14, 2019

CRM-18792 - CRM_Core_Theme - Add helper for loading CSS files from th…
…emes

CRM-18792 - CRM_Core_Resources - Load civicrm.css through theme system

CRM-18792 - Rename `CRM_Core_Theme` to `\Civi\Core\Theme`

CRM-18792 - Civi\Core\Theme - Remove statics

WIP

CRM_Core_Resources::addCoreStyles - Revert change

CRM-18792 - addStyleFile - Always pass through to theme. Support fallback.

Rename `Civi\Core\Theme` to `Civi\Core\Themes`

The class manages a list of themes -- not just a single theme.

CRM-18792 - Add org.civicrm.demotheme

CRM-18792 - Add uncommitted test files (`Civi\Core\Themes`)

CRM-18792 - Fix regression in CRM_Core_ResourceTest

CRM-18792 - Theme naming - Use prefix '_' for hidden themes

This cleans up a few things:

 * Previously, there was a special case for using FALLBACK in `search_order`.
 * If you're creating a multitheme extension, you may want to define a base theme
   (which is extended by the others). Previously, you were required to show this
   base theme as a user-selectable option. Now, it can be hidden.
 * There was a bug where `resolveUrl()` would sometimes call the wrong callback.
   (It used resolver for `$active` instead of `$themeKey`.)

CRM-18792 - Themes - File overrides and excludes should use same naming

Previously, when using `addStyleFile($cssExt,$css$file)`, the file overrides
and exlcudes would combine them differently e.g.

 * For `addStyleFile('civicrm','css/bootstrap.css')`
   * Override `css/bootstrap.css`
   * Exclude `civicrm:css/bootstrap.css`
 * For `('org.foo.bar','css/bang.css')`
   * Override `org.foo.bar-css/bang.css`
   * Exclude `org.foo.bar:css/bang.css`

Now, they use the same notation:

 * For `addStyleFile('civicrm','css/bootstrap.css')`
   * Override `css/bootstrap.css`
   * Exclude `css/bootstrap.css`
 * For `('org.foo.bar','css/bang.css')`
   * Override `org.foo.bar-css/bang.css`
   * Exclude `org.foo.bar-css/bang.css`

"Display Preferences" - Add the `theme_backend` and `theme_frontend` settings

hook_civicrm_activeTheme - Allow extensions and CMS modules to choose active theme

CRM_Utils_Hook::themes() - Tweak docblock

Civi\Core\Themes - Move cache from `short` to `long`

Remove tools/extensions/org.civicrm.demotheme

Fix merge ahem errors

@eileenmcnaughton eileenmcnaughton force-pushed the totten:master-theming-rb branch from ebb1e2c to d89d254 Jun 15, 2019

@kcristiano

This comment has been minimized.

Copy link
Contributor

commented Jun 15, 2019

@eileenmcnaughton

This comment has been minimized.

Copy link
Contributor

commented Jun 15, 2019

I've added merge on pass - we looked at this at the sprint and there was agreement this takes us forwards without potentially solving everything.

There is a docs PR & a civix PR and I'm also hoping @mattwire will get his head around the steps involved to add PRs to the listing & update themes he is involved in to adapt.

From discussions etc this is greenfields & non breaking and has been considered extensively over a long time. There is also enthusiasm to work with it & iterate if need be

@eileenmcnaughton

This comment has been minimized.

Copy link
Contributor

commented Jun 15, 2019

@seamuslee001

This comment has been minimized.

Copy link
Contributor

commented Jun 15, 2019

Test fail looks unrelated to me merging

@seamuslee001 seamuslee001 merged commit b203406 into civicrm:master Jun 15, 2019

1 check failed

default Build finished.
Details

Product maintenance automation moved this from requires more work to be reviewable to Pending Merge Jun 15, 2019

review board automation moved this from needs agreement on tech approach to done Jun 15, 2019

@eileenmcnaughton

This comment has been minimized.

Copy link
Contributor

commented Jun 15, 2019

woot!

@eileenmcnaughton

This comment has been minimized.

Copy link
Contributor

commented Jun 15, 2019

@seamuslee001 I was gonna check if the related docs & civix were merged - but the link is broke :-)

@seamuslee001

This comment has been minimized.

Copy link
Contributor

commented Jun 15, 2019

I've just merged the docs one but the civix one totten/civix#156 needs Tim to do the merge

totten added a commit to totten/civicrm-core that referenced this pull request Jun 15, 2019

CRM-18792, dev/core#378 - Catch-up on settings metadata
This is a follow-up to civicrm#12929.  In the interim between the writing and
merging of the PR, the version numbers changed.

@totten totten deleted the totten:master-theming-rb branch Aug 8, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.