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

dev/user-interface#27 - Define a "bootstrap3" bundle (skeleton) #18354

Merged
merged 3 commits into from
Sep 5, 2020

Conversation

totten
Copy link
Member

@totten totten commented Sep 4, 2020

Overview

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 is a step toward https://lab.civicrm.org/dev/user-interface/-/issues/27. It extracts/polishes some parts of the exploratory branch #18190.

Before

There is no official contract between (a) extensions which provide Bootstrap CSS implementation and (b) extensions (or core pages) which consume the Bootstrap CSS implementation.

In the case of, say, Shoreditch, their work-around is to activate the file on every pageload, which creates other problems. Some implementers create targeted overrides just for Shoreditch's bootstrap.css, but this creates problems with mixing/matching. (Ex: If a CMS integrator writes an override to exclude Shoreditch's bootstrap.css, then that exclusion is unlikely to apply to Greenwich's bootstrap.css.)

After

The bundle name bootstrap3 represents a contract between different parties.

  • For application-pages, they may request it via Civi::resources()->addBundle('bootstrap3');
  • For theme-extensions, they may provide it via hook_civicrm_alterBundle.
  • If a site-build provides bootstrap3 by some means that we're unaware of, then the entire bundle can be disabled. (Currently, this could be done with a late-priority listener for hook_civicrm_alterBundle which calls $bundle->clear() . We should probably expose this as a UI option as well.)

Technical Details

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 to be provided by an extension (e.g. #18190 shows this being used with a core extension "Greenwich").

NOTE: Why is bundle called bootstrap3 instead of just bootstrap? The bundle name represents a contract between app-dev and theme-dev, but there's been confusion/debate about whether to interpret "bootstrap" as "v3" or "v4" or "v5" or "pick your own version" . This approach means:

  • The contract is unambiguous.
  • Someday, we might leap-frog to (say) bootstrap6 or bootstrap7. It will be possible for there to be a period where (say) bootstrap3 and bootstrap7 coexist (identified and activated separately).

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.
@civibot
Copy link

civibot bot commented Sep 4, 2020

(Standard links)

@eileenmcnaughton
Copy link
Contributor

Oh this looks like a good step - are we at a point where we can add a docs PR?

@eileenmcnaughton
Copy link
Contributor

@totten this doesn't have a test but the last PR had many - I assume it is 'ok-without-test' as it's part of a bigger sequence with appropriate cover....

@totten
Copy link
Member Author

totten commented Sep 4, 2020

Yeah, I'm biased on this, but I also lean 'ok-without-test'... Most the of the assumptions that it's based on (re:Resource Collections) were tested aggressively in the earlier PR. For the distinctive parts here, it's hard to think of a good way to unit-test (at least in our current framework) You'd maybe have to mock a theme-ext, implement hook_alterBundle, and then do an E2E test on the output of (eg) the Api4Explorer page to ensure that it loaded CSS from the mocked theme-ext. I'd love if we had that, though maybe a bit of rabbit-hole at the moment?

@eileenmcnaughton eileenmcnaughton added ok-without-test merge ready PR will be merged after a few days if there are no objections labels Sep 4, 2020
@eileenmcnaughton
Copy link
Contributor

I'm merging this - it's a pretty small chunk of a big picture. My main reservation is the documentation update but I don't think that has to be 'this PR'

@eileenmcnaughton eileenmcnaughton merged commit 587db57 into civicrm:master Sep 5, 2020
@totten totten deleted the master-bs3b branch September 8, 2020 03:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
master merge ready PR will be merged after a few days if there are no objections ok-without-test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants