CRM-18792 - Create CSS theming subsystem #8523
Conversation
totten
commented
Jun 7, 2016
•
totten
commented
- CRM-18792: Allow extensions to define CSS themes
totten
changed the title
totten
force-pushed
the
master-theming
branch
4 times, most recently
from
8b58450 to
86de242
Compare
|
So in this case, the test failure really is indicating a change in behavior. Consider the call: Civi::resources()->addStyleFile('civicrm', 'css/bootstrap/does-not-exist.css');The file
It's not really clear what behavior makes the most sense here (throw an exception; add the file anyway; skip it). In absence of a clear reason to change, I'll update PR to match the old behavior. |
totten
changed the title
|
@AkA84 @agh1 @jamienovick @kcristiano , I've drafted a chapter for https://docs.civicrm.org to go along with this: https://github.com/totten/civicrm-dev-docs/blob/master-theme/docs/theme.md The docs have a some big blank spaces for describing the actual CSS of |
|
Ill perhaps leave this to the more technical of you to review but thanks Once it all looks good and you guys are happy I'll draft a blog post for J On behalf of Compucorp Ltd Jamie Novick / Director Tel: +44 (0)207 096 3336 / Mob: +44 (0)775 123 8875 Skpe ID: jamie.novick This email is confidential and is intended for the addressee only. If Unless expressly stated to the contrary, the content of this e-mail does All reasonable efforts have been made to confirm that this e-mail and _ On 21/06/2016 05:37, Tim Otten wrote:
|
totten
force-pushed
the
master-theming
branch
2 times, most recently
from
6e0f290 to
885af19
Compare
|
Hi @totten , I’ve read the documentation and I’ve got a couple of questions Are you going to add a theme-specific class to the main container tag? Something like:
<div class="crm-container crm-theme-shoreditch">
</div>
<div class="crm-container crm-theme-greenwich">
</div>
as discussed during the sprint I think this would provide a nice way to write ad-hoc css rules just for a specific theme I thought we were going to have a subfolder for the theme even if it’s the only theme in the extension? For example, from the "Packaging section": this way a theme author would have to learn just a single, coherent folder structure for both single and multi theme extensions, and I think at that point you could get rid of the // FILE: newyork.php
function newyork_civicrm_themes(&$themes) {
$themes['astoria'] = array(
'ext' => 'org.civicrm.theme.newyork',
'title' => 'Astoria'
);
$themes['wallstreet'] = array(
'ext' => 'org.civicrm.theme.newyork',
'title' => 'Wall Street',
);
}// FILE: newyork.php
function newyork_civicrm_themes(&$themes) {
$themes['astoria'] = array(
'ext' => 'org.civicrm.theme.newyork',
'title' => 'Astoria',
'search_order' => array('astoria', '_newyork_common_', '_fallback_'),
);
$themes['wallstreet'] = array(
'ext' => 'org.civicrm.theme.newyork',
'title' => 'Wall Street',
'search_order' => array('wallstreet', '_newyork_common_', '_fallback_'),
);
$themes['_newyork_common_'] = array(
'ext' => 'org.civicrm.theme.newyork',
'title' => 'New York (Base Theme)',
);
// Note: "_newyork_common_" begins with "_". It is a hidden, abstract
// theme which cannot be directly activated.
}What do you think? In the "Packaging" section we have
In the "Advanced:
Two questions:
Basically I’m not sure I understand what’s the use case of
Are we going to have a this stage a parent/child theme mechanism? Thanks! |
|
(Note: I've tried to re-order questions to get the simpler ones out first.)
Well, if you compare to the call
That's what
Thanks for catching/remembering! I'll try to do something for the container tag. A few thoughts:
TBH, I feel like we got into bike-shed territory on this, but I think I understand the design you described, so I've tried to make a gist of the different approaches (with some supportive/opposing arguments; feel free to fork/edit if I've misinterpreted or misrepresented). https://gist.github.com/totten/cf80ebf266185cb0b9fbd7f463ad2515 Personally, I'm suspicious about getting the semantics right (how parent/child should work; how to override CSS vs how to supplement CSS; how to convey the semantics to downstream devs; etc). I'd like to get single theme working before getting really invested in multitheme, so my gut here says "keep it simple/less is more/let downstream iterate on it". If I could rate each approach (0=awful, 5=wonderful):
How would you rate them, and how much do you think it matters? Perhaps a straw-poll (pinging a few more devs) would be useful?
The scenario I had in mind was this: Alice is a theme developer who's written a Drupal theme based on Bootstrap and SASS. She wants the theme to work nicely in Civi, and it would mostly work (because it defines all the standard Bootstrap classes), and she's willing to add a few extra CSS classes or SASS directives to satisfy Civi's style-guide. All her code compiles into one Of course, this is one scenario, and I think you're describing another -- Bob is a Drupal themer who publishes a Bootstrap-based theme. Carol is an admin who installs both Bob's theme and installs CiviCRM. In this case, Bob's theme is close (it already loads IMHO, the current PR allows a themer to address any of those scenarios, e.g.
I like that this allows each scenario, but it does put a higher burden on the administrator -- when configuring For reference... well thinking about this... I drew a couple diagrams which might be interesting. The first is a Venn diagram of the different CSS vocabularies: https://docs.google.com/drawings/d/1UeTddVvmHBCIEwQ-NoNTWoNWgvs1Nvq1kdwe2tif1xA/edit?usp=sharing and the second shows the ways to package up themes: https://docs.google.com/drawings/d/15wfwOCJ6YsThnyP7TATC1CZt9YCyyd-o1RuzfD57ZSI/edit?usp=sharing Not sure yet how to make the admin experience smoother, so will try to sleep on that. |
The class manages a list of themes -- not just a single theme.
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`.)
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`
Theme markerYou're right, apologies I still get confused sometimes by what's Civi territory and wha't CMS territory! I think that’s fine then to apply the theme marker just to File structureWell tbh for me the ratings would be completely reversed :D
To me the gist you made shows that option no.2 is really the cleanest and simplest. Whether you have 1 or 4 variations of the same theme, the structure stays consistent and the configuration code remains bare-minimum. I’m not sure what your worries are about how things might change in the future, which I understand is your main concern about having a so strict contract between the theme system and the theme builder about folder structures but certainly you know more about how things evolve in CiviCRM than I do, so definitely you have a clearer picture for making the final decision If I have to choose between 2 and 3, I would choose 2 as it seems a little less..obscure (to me at least) in how to use it, and allows for more control in case you have multiple themes/variations in your extension (you can explicitly specify the folder for each variation, and you can define the search order). Maybe I would swap "search_order" for "parent" as suggested below? (didn’t read yet your comments on that) Parent/child themes(bear in mind that this is merely a brainstorming thing! Might be a bit too much for the first iteration of the theming system, but thought might be useful to at least bring up this possible feature) I think What we were briefly discussing during the sprint was to have a system where a theme extension can reference another theme extension as its own parent. So let’s say you have // In org.civicrm.theme.europe/europe.php
$theme["europe"] = array(
"ext" => "org.civicrm.theme.europe",
"title" => "Europe",
);
// In org.civicrm.theme.italy/italy.php
$theme["italy"] = array(
"ext" => "org.civicrm.theme.italy",
"title" => "Italy",
"parent" => "europe"
);If the current theme is set to be If the current theme is set to be (if then CiviCRM could concatenate the files it would be even better) Two notes about this solution:
Come to think of it, I wonder if the function newyork_civicrm_themes(&$themes) {
$themes["_newyork_common_"] = array(
"ext" => "org.civicrm.theme.newyork",
"title" => "New York (Base Theme)",
);
$themes["astoria"] = array(
"ext" => "org.civicrm.theme.newyork",
"title" => "Astoria",
"parent" => "_newyork_common_"
);
$themes["wallstreet"] = array(
'ext' => "org.civicrm.theme.newyork",
"title" => "Wall Street",
"parent" => "_newyork_common_"
);
}CMS+Civi themes approachI feel a bit confused by this whole part of the conversation I’m afraid. The reason being that, as far as I know, up to this point the only scenario that was considered was what you now describe as the “Overload approach”: the CiviCRM theme includes vanilla bootstrap (with variables tweaked to match the new design, and resets to counteract the CMS theme) + overrides to already existing components (i.e.: slightly changes in appearance for panels, tables, etc) + styling for CiviCRM specific components for which there is no equivalent in Bootstrap. I don’t remember conversations about a possible interplay between the CMS and Civi themes, and actually I think I recall conversations with @jamienovick where the idea was something like having Civi shipping with a “neutral” CMS theme, so that it would not interfere with the CiviCRM one. Actually I think during CiviCON we were even talking about this idea of using the style guide extension as to survey, between Civi admins, how many CMS themes were incompatible with If my recollections are wrong, or if I misinterpreted part of the conversation, apologies, but I believe it’s better to have this matter clarified before we get into the details of your scenarios! |
|
@AkA84 thanks for the comments. Will try to consider them more. Question for clarification -- in describing the relationship between https://github.com/civicrm/civihr/blob/develop/org.civicrm.bootstrapcivihr/ vs https://github.com/civicrm/civihr/blob/develop/org.civicrm.bootstrapcivicrm/ , I'm a little confused about whether it's doing:
|
|
@totten I didn't expect you to reply so quickly :D I'm still in the middle of adding my replies to the remaining comments, bear with me :) |
|
Hi @totten , I finished to write my replies in the comment above ^ |
|
re: CMS+Civi themes approach Im not 100% sure what the debate is about but I think we should have a "CIVICRM admin Drupal theme" that is neutral and simply sets out that standard regions. Then all the clever stuff is done in the CiviCRM theme layer itself. nb. Is the public/private CiviCRM pages being considered? I think it is important that we avoid putting out default styles for public pages (or are easily able to switch these off!). |
|
Thanks for all this. Trying to make Civi play nicely with an unknown CMS + theme is tricky ... I don't have many strong opinions about how it's done (though I'll probably complain at the end of it all anyway), but here is what I see as the key challenge: 2 conflicting and not-well-defined needs. Need 1. Out of the box, the Civi 'admin' experience looks reasonable and functional for both desktop and mobile. So for those screens, civi can be opinionated and aggressive, and anyone who wants to change it can expect to do some work. Civi will need to provide all the bootstrap bits and assume relatively little from the CMS. Need 2. It's possible and not to painful to make the civi 'public' experience relatively 'seamless' with the rest of the site. I don't expect plug and play here, but the current situation where the default can be hugely jarring and it's often a royal pain to get it even reasonably non-jarring is a big pain point, especially for (e.g.) new themers coming from Drupal. For this to work, Civi has to be very unopinionated and flexible, and it would seem to me that in this case we don't want to be providing all the bootstrap bits, but only generating html with bootstrappish classes that look nice in a CMS bootstrap theme but isn't impossibly nasty in another theme. In particular - what about when the CMS is using a slightly different version of bootstrap compared with the Civi version? I know we can just do some clever bits so they co-exist, but that's not very nice integration and makes theming extra complicated. ... The distinction between 'public' and 'admin' is one vague area, though the civitheme in Drupal seems to have a reasonable working definition. I'm not sure if/how Wordpress or Joomla handle this. I've also recently waded through a similar discussion for drupal 8, which is nicely summarized here: https://www.lullabot.com/articles/a-tale-of-two-base-themes-in-drupal-8-core This problem is slightly different, but some kind of clarity about the two separate needs and how they are resolved with this solution would ease my mind. |
|
Coming back to this after so long certainly helped me with approaching it with a fresh mind! Biggest problem for me was that I couldn't understand the So ok, this solution takes into account all three main scenarios, which is good, but I was wondering about the pros and cons of each of them. And also we should agree and be clear on what approach the initial theme ("shoreditch") should follow. (note: this considers for now only the admin part, i'll touch on the 'public' one later) All-encompassing approachPros: Given that you've disabled the Civi admin css and you're relying entirely on the CMS admin theme, you don't have to worry about the two conflicting with each other. Cons: How common would this be? Would it be common for themers to package their Drupal/Wordpress/Joomla themes with extra bits and pieces specific for Civi? Would these themes still be distributed as generic CMS themes or would they be more niche, just for the Civi community? Also this seems a lot of work for the initial theme, since we would have to create 3 versions of it for the 3 CMS, not to mention that in this case it would be a full CMS admin theme, not just a Civi theme Polyfill approachPros: It's a great way to leverage already existing Bootstrap themes out there, so that you don't need to re-invent the wheel for the common Bootstrap elements and you can focus on the CiviCRM custom elements only. I imagine one would package a polyfill theme extension with info.xml explicitly saying that the extension requires a specific CMS theme to be enabled, and that would be it. Given that the theme has a very specific CMS theme as a dependency, you can either avoid the conflicts between the two altogether or know exactly what they are and fix them Cons: You are introducing a big external dependency on your Civi theme, and as @adixon says, probably Civi, at least for the admin part, should assume very little from the CMS and be very opinionated. For the initial theme this would mean either finding a good CMS admin theme candidate or, again, creating one our own Overload approachThis is the approach that we are taking now with the theme we are developing here at Compucorp and also the approach that was mostly discussed in previous conversations (see my previous comment) Pros: With this approach you author the Civi theme without having to make any assumptions on what CMS theme the admin has enabled on the site, which means you have no dependency. Cons: Since the Civi theme is completely independent, it will have to reset any interfering style coming from the CMS, and given that you can't know what theme is enabled, it would be tricky to say the least to be sure that your theme will work fine in every scenario (Unless you introduce a dependency in the form of a "blank" CSM admin theme?) Does this sound correct in relation with what has been discussed so far? Or did I miss/misunderstand something? About the public part of the site, I'm afraid I'm not yet very familiar with how Civi works in that regards? Does it inject its css in it (if i take a look at the demo I don't see any sign of that)? What are your biggest concerns about it? Hope this was useful somehow, at least to keep the conversation going! |
|
@totten this is now officially the oldest PR & is thoroughly stale. Is anything happening with it. I do think that closing the PR & just linking to it from the JIRA is a good way to deal with good intentions / good ideas that didn't quite get somewhere |
|
@totten this is now the oldest ticket! What part of this needs merging? I know shoreditch is already released... |
|
This is a year old and Shoreditch is progressing without it. Let's close. |
|
Just linking to what I think is the related feature request, https://lab.civicrm.org/dev/core/issues/378 - where discussion about this change can continue. |
