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

(do-not-merge) Bootstrap support via Greenwich core extension #18190

Closed
wants to merge 15 commits into from

Conversation

totten
Copy link
Member

@totten totten commented Aug 18, 2020

Overview

This is an exploratory branch to demonstrate a more modular and nuanced approach to loading Bootstrap3 CSS. Its aim is to provide breadth and to show the connection between different pieces. The explanation is grouped into a few subtopics, each of which would likely be its own PR and possible revisions.

See also: https://lab.civicrm.org/dev/user-interface/-/issues/27

Resource collections

NOTE: After discussion of this exploratory branch, this portion was split out as #18247.

How do you manipulate a list of related assets - such as a combination of CSS+JS+data that must be loaded together?

There are three places where you might add, inspect, or tap-into such a list. These three build on top of each other, and they are broadly similar, but there are several technical differences. A handful of differences are important, but many of the differences are arbitrary.

Criterion CRM_Core_Resources hook_coreResourcesList CRM_Core_Region
Method style addFoo('bar') $arr[] = 'bar' add(['foo' =>'bar'])
IDE Discovery Great Poor OK
Alterability Poor Great OK
Singleton/Lego Singleton-like Singleton-strict Lego-like
Content ID None URL Name (if given) or URL/Number
Content type: ScriptFile Yes No No
Content type: ScriptURL Yes Yes Yes
Content type: ScriptCode Yes No Yes
Content type: StyleFile Yes No No
Content type: StyleURL Yes Yes Yes
Content type: StyleCode Yes No Yes
Content type: Setting/Var Yes Yes No
Content type: Strings Yes No No

The idea here is to (a) create a model which combines most of the above characteristics, (b) make the model more reusable, and (c) retrofit that model as far as we can into the existing system. The unified concept of a "resource collection" captures all the similarities between them. Of course, there are still a few differences - and so we have different types of collections:

Screenshot from 2020-08-18 16-14-02

The CollectionTrait and CollectionInterface are central to this:

  • Its add(), update(), and get() methods are interoperable with add(), update(), get() from CRM_Core_Region
  • Its addFoo() methods are interoperable with the addFoo() from CRM_Core_Resources. They are all thin wrappers for add().
  • It supports all content-types that appeared in CRM_Core_Resources, CRM_Core_Region, or hook_coreResourcesList.
  • It supports all per-resource options that appeared in CRM_Core_Resources or CRM_Core_Region (such as weight, region, name, disabled).
  • It supports alterability like hook_coreResourcesList, but its format is different. It allows more content-types, and it provides helper methods like get($name), getAll(), filter($func), find($func), and clear().
    • Note: For backward-compatibility, we still fire hook_coreResourcesList. However, it doesn't have the full API or full content-model. The equivalent with a full API is hook_alterBundle.
  • The behavior of a collection can be tuned via property string[] $types (white-list of allowed resource-types) and array $defaults (list of key-values to apply to all new resources).

Both Region and Bundle are based on CollectionTrait, so they are very similar. The difference is that:

  • A Region is a concrete section of the HTML document - so it can be rendered to HTML.
  • A Bundle is an unattached list - it is not active until you add/merge into something concrete. (ex: Civi::resources()->addBundle($myBundle)).
  • The whitelist of allowed resource-types (scriptUrl, scriptFile, markup, etc) is tighter in a typical Bundle than in a typical Region.

Bootstrap3 bundle

NOTE: After discussion of this exploratory branch, this portion was split out as #18354.

If a page is written for the Bootstrap3 CSS vocabulary, then it should activate the corresponding bundle:

Civi::resources()->addBundle('bootstrap3');

The bundle is a mix of different kinds of assets (e.g. CSS and JS).

Observe that different stakeholders interact with this bundle in different ways by :

  • Application-pages (in civicrm-core or extensions) activate the bundle (but the implementation is opaque to them).
  • Themes (in extensions) supply content for the bundle (but they are not responsible for deciding when to load it).
  • CMS integrations/addons may veto the bundle in whole or in part (without knowing the specific pages that use the bundle -- and without knowing whether the files are called bootstrap.css, bootstrap3.css, greenwich.css, greenwich-bootstrap.css, or asset-builder?asdf1234).

This particular bundle is not really provided by civicrm-core -- core is just the middle-man who gives the name bootstrap3. The default content is provided via the Greenwich theme/core-extension.

NOTE: Why is bundle called bootstrap3 instead of just bootstrap? Two reasons:

  • The bundle name represents a contract between app-dev and theme-dev. This name removes ambiguity in that contract.
  • Someday, we might leap-frog to (say) bootstrap6 or bootstrap7, which will not be interoperable with bootstrap3.

Greenwich extension

NOTE: After discussion of this exploratory branch, this portion was split out as #18465.

Currently, civicrm-core has a built-in theme called "Greenwich". However, it is enmeshed with the rest of the core code. With this patchset:

  • It formally becomes its own extension - so it's better role-model for how to write a theme.
  • It is auto-enabled and hidden - in the same way that sequentialcreditnotes is. So it is always available.
  • It includes a copy of BootstrapCSS that is compiled to work within the container #bootstrap-theme (ie the same container used by Shoreditch). The compilation uses another extension, csslib.

@civibot
Copy link

civibot bot commented Aug 18, 2020

(Standard links)

@civibot civibot bot added the master label Aug 18, 2020
@totten totten force-pushed the master-res-reg-rb branch from b5f178f to 1a0b777 Compare August 19, 2020 00:01
@seamuslee001
Copy link
Contributor

Test fails look related @totten

@totten totten force-pushed the master-res-reg-rb branch from 1a0b777 to 4085bfc Compare August 19, 2020 01:35
@totten
Copy link
Member Author

totten commented Aug 19, 2020

@seamuslee001 Ah, thanks. I'm pushing an update for that.

@colemanw @eileenmcnaughton @seamuslee001 You should be able to check out this PR and see the APIv4 Explorer using Greenwich's BootstrapCSS on a new/clean build. This branch incorporates #17913 - and it also adds several other patches. However, within each subtopic, I think there are points of revision/discussion. But if the overall structure seems sane, then I can split it out into smaller PRs.

@eileenmcnaughton
Copy link
Contributor

@totten can you rebase when you get a chance?

@totten
Copy link
Member Author

totten commented Aug 20, 2020

@eileenmcnaughton Well, I could, but... this is an exploratory/demonstrative branch that aims to give broad sense of the cumulative result of multiple PRs. It's not supposed to be merged as-is, and it's quite prone to staleness. Maybe it makes more sense to put the effort into filing a few of smaller PRs?

@eileenmcnaughton
Copy link
Contributor

@totten yeah - it was mostly that I thought about pulling it down to look & then saw it was stale so I didn't - but maybe it's better if we have that call that was scheduled earlier & discuss / demo

@totten
Copy link
Member Author

totten commented Aug 20, 2020

Agree, we should do a little discuss/demo.

@totten
Copy link
Member Author

totten commented Aug 24, 2020

@colemanw @eileenmcnaughton @seamuslee001 - I've taken out the parts which deal with just "Resource Collections" / "Resource Bundles" and put that in #18247. That's somewhat smaller (e.g. doesn't touch on the Greenwhich/Bootstrap stuff). It's also rebased to match some incidental changes, and I've added a few updates/fixes as well as notes+suggestions on QA.

@eileenmcnaughton
Copy link
Contributor

@totten please rebase

@totten
Copy link
Member Author

totten commented Sep 4, 2020

Right, all the "Resource collections" stuff has been merged in its own PR. That still leaves the "Bootstrap3 bundle" and "Greenwich extension".

Note that the approach to the Greenwich's compilation step is changing (discussed more in https://lab.civicrm.org/dev/user-interface/-/issues/27), so this shouldn't be merged. But it's still useful for seeing the parts put together.

@eileenmcnaughton eileenmcnaughton changed the title (EXP) Bootstrap support via Greenwich core extension (do-not-merge) Bootstrap support via Greenwich core extension Sep 4, 2020
The Api4Explorer page has a neat trick - if (by any means) you manage to
have Bootstrap3 active within the body of this Civi page, then it will
display a warning.

This patch moves the trick to be part of the `bootstrap3` bundle, which
means that it will be applied to any page where `bootstrap3` is activated.
@totten
Copy link
Member Author

totten commented Sep 4, 2020

@eileenmcnaughton @colemanw - I've extracted the "Bootstrap3 bundle" parts as #18354.

For a more proper rendition of the "Greenwich extension" section, that's going to depend on some composer work that will have to go into another repo, so we won't see that part in civicrm-core.git until the substantial work for that is published.

@seamuslee001
Copy link
Contributor

@totten this needs another rebase

@totten
Copy link
Member Author

totten commented Sep 14, 2020

@seamuslee001 @colemanw @eileenmcnaughton - At this point, I believe everything from the experimental PR has been extracted/polished with proper PRs, so I'm gonna close this one.

The new Greenwich-specific PR is #18465. Note that it has the one major change, shifting from AssetBuilder to a composer-based pre-compilation.

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.

3 participants