backdrop-contrib / bootstrap5_lite

GNU General Public License v2.0
2 stars 6 forks source link

Bundle Bootstrap #16

Closed docwilmot closed 1 year ago

docwilmot commented 2 years ago

Currently we're loading Bootstrap, Bootswatch and Font Awesome via CDN. We could bundle the files instead.

bugfolder commented 2 years ago

Yes, I did this on Bootstrap Lite in issue 31; implemented in https://github.com/backdrop-contrib/bootstrap_lite/pull/46, https://github.com/backdrop-contrib/bootstrap_lite/pull/50 (bugfix), https://github.com/backdrop-contrib/bootstrap_lite/pull/53 (further bugfix).

Happy to do the same on BS5L (hopefully in a single PR this time) if you're amenable.

docwilmot commented 2 years ago

That would be great if you would.

bugfolder commented 2 years ago

I've got this now working, PR to follow. Putting it together raised some similar issues in Bootstrap Lite.

I've added support for Bootstrap, Bootswatch, and Font Awesome bundled libraries. While I was doing that, I noticed that Font Awesome now has a version 5 (previously only two 4.x versions were supported), so I also added support for Font Awesome 5. As we did with Bootstrap Lite, the bundled version is the most recent version, so it's FA version 5.

Bootstrap also changed how they handle their own icons going from 3 to 5. Bootstrap 3 used a built-in font, "Glyphicon Halflings", which Bootstrap 5 has a separate library for Bootstrap Icons. So that, too, now supports either CDN or bundled versions.

Between BS3, BS5, and FA4/5, there are 3 different ways of creating icons (only the latter two are supported in this module). So to reduce potential confusion, I added some discussion and examples to the README file.

Last, there's an issue of how the files are organized within the theme. As in https://github.com/backdrop-contrib/bootstrap_lite/issues/54, I've put each of Bootstrap, Bootstrap Icons, Font Awesome, and Bootswatch bundled libraries into their own eponymous directories, making the first 3 identical to their downloads for easy future drop-in upgrades.

I left in the unminified files (to keep the libraries drop-in-able, and to possibly support future debug setting to use unminified files). This has a cost, though, of making the overall theme size pretty large (34 MB). (Almost half of that is Font Awesome.) At the moment, I'm inclined to live with the size to keep the organization clean, but am open to alternatives.

PR to follow.

bugfolder commented 2 years ago

@docwilmot, after thinking about the size issue a bit more, I've got another idea. Rather than having two branches, how about if we keep one branch that doesn't contain the bundled libraries, but that will offer them in the theme settings if they're present in the theme. We add the bundled libraries as an attachment to a Github Wiki page for this module, which will contain the bundled libraries and instructions on how to download and where to put them. And then the README file can just contain a pointer to the Wiki page for those who want to use the bundled libraries.

An example of this is this Ubercart Wiki page, which provides some downloadable files useful in upgrading.

What do you think?

docwilmot commented 2 years ago

That would be fine I suppose, but the advantage of a branch is having everything versioned. I dont know how they got files attached to the Wiki, I dont see an upload button anywhere, but if t is versionable from there then we could do that.

bugfolder commented 2 years ago

It turns out that the Wiki is its own repo, which we can clone, edit locally, and push things to it—like a files/ directory. So we could upload the "bundled" libraries to that directory, and they'd live there, and also they'd be present in the commit history for the repo.

Another potentially nice thing about putting the bundles in the Wiki is that if Bootstrap 5 and Font Awesome 5 release newer versions, we could add support for the newer versions in new installations by adding more versions of the bundled library to the Wiki page, while keeping the old ones there as well for continuity/backward compatibility.

This page explains how that works. I've set this up for the Ubercart wiki (that's where the stub files reside). I could do this as well for this module's wiki.

docwilmot commented 2 years ago

I'm thinking we could be overthinking this. We should just bundle the individual min.css/js files and the couple fontawesome icons in use as a minimal effort. It would end up being at most 10 files or so.

bugfolder commented 2 years ago

A difficulty with that is that although we know what FontAwesome icons we might be using, we have no way of knowing what icons users might be using and/or expecting to have accessible if they selected the "bundled" option.

I think I recall @herbdool commenting that he would like to see the bundled libraries, so perhaps he could chime in on this issue?

docwilmot commented 2 years ago

Hence the "minimal" part. It seems that Fontawesome is the largest library and yet least absolutely necessary OOTB. We can add instructions for downloading the full library for those who need additional icons. Everyone else would get full functionality, wihtout having to download and build multiple libraries.

herbdool commented 2 years ago

@bugfolder I don't recall saying that, though who knows. I forget lots of things

docwilmot commented 2 years ago

Actually I hadn't really considered that this theme supports 6 versions of Bootstrap, each with a different set of CSS and JS files, for each of 25 Bootswatch themes.

We would need to decide to support only one version locally, since that would be way too many combinations.

But then, if we're providing 5.0.1 locally OOTB, and Bootstrap 5.0.2 comes out, what to do then? We cant bump off the 5.0.1 local files and start supporting only the latest, cause there'll be people already depending on that. We'll have to make a new release of the theme for every new version, each supporting one Bootstrap version, right? I think that would be too much for me.

I think we either leave this, and maybe provide instructions for users to compile their own local versions, or come up with some automatic solution to download desired assets.

And thats not even considering FA versions.

@bugfolder I'm only now seeing this as a problem, but perhaps you've thought it through. Did you have a long term vision for supporting local files?

bugfolder commented 2 years ago

Did you have a long term vision for supporting local files?

Well, here's what I was thinking. The driving fact is, to offer FontAwesome bundled OOTB means a huge theme download, and I don't see any way around that. So the OOTB offering has to be not bundled to keep the file size tractable.

But there are certainly people who prefer to have a locally bundled library. (I do, when I'm working on airplanes with no internet.) So we make it possible by saying in the documentation that users can download and install the bundled libraries. And to keep the README file from getting big, complicated, and full of information that's not of interest to people who don't care about bundling, we put the details of the bundling on a Wiki page for the GitHub module.

And for those who want their libraries local, we create a nicely zipped archive of the bundle files (say, version 5.0.1) so that we can tell them "download this, unzip it, and drop the folders into the theme root folder."

We label the bundles on the Wiki with their current version. If BS/FA come out with never versions, we can add new bundles to the Wiki while keeping the older ones available.

How's that sound?

docwilmot commented 2 years ago

Ahh yes, you did mention using the Wiki instead of bundling them into the package. Good plan. As a wise man once said

I forget lots of things

So then 8 different zips would need to be provided:

Sound right? (edited)

bugfolder commented 2 years ago

I don't think we need to provide that much variety. I envision three options for users:

Easiest

Use the CDN versions of Bootstrap, Bootswatch, Bootstrap Icons, and FontAwesome. You can choose from the versions offered by the CDNs (since it's no extra effort on our part to offer them).

(Relatively) Easy Bundled

Download the (single) zipped directory from the Wiki, unzip it, and drop the resulting folders into the theme root file. The recommended bundled version will always be the latest versions of the library. Note, though, that you will need to reinstall the bundled libraries if you upgrade the theme. (Oooh, that could be a deal-breaker there. I'd hate to think that a simple bugfix release makes the user go through the whole rigamarole of installing the bundled files again. Not really "bundled", is it?)

Bundled, for Experts

If you really want a locally installed version but want to use one of the multiple options of Bootstrap (e.g., the RTL version, just the grids version, unminified version for debugging, etc.), download the full library from the Wiki (or, perhaps, we just give the links to download directly from GetBootstrap.com), and do your own surgery.

Up until a few minutes ago, I thought that the above would be a reasonable approach. But the problem of having module updates wipe out the locally bundled library seems pretty bad. Back to thinking about it.

bugfolder commented 2 years ago

Maybe we just drop FontAwesome from the theme, and recommend that users who need it use the contrib FontAwesome module? It was FontAwesome that was causing most of the file size problems.

We could similarly create a separate Bootstrap Icons contrib module and leave that out of the theme, and that would save another chunk of file size.

That would leave the 25 Bootswatch CSS files, which still total to 4.7MB if bundled.

docwilmot commented 2 years ago

Realistically though, why bother with versions < 5 in this theme? We bundle only Bootstrap 5 files with this theme and leave the rest to Bootstrap Lite.

bugfolder commented 2 years ago

Agreed! I was just keeping it in my PRs because that's the way I found it in this theme when I started tinkering.

bugfolder commented 2 years ago

OK, having thought about this overnight, here's my current thinking of approach:

How's that sound?

jenlampton commented 2 years ago

How's that sound?

I think it sound great. I'm a fan of bundling things (to make it easier on those installing) but also happy to let individual modules add bonus features like font awesome :)

bugfolder commented 2 years ago

I have updated this PR to unbundle Bootstrap Icons and FontAwesome, adding instructions to the README to lead people to the corresponding contrib modules.

I also removed two references to font-family: fontawesome in overrides.css, which had been added to replace the Glyphicons-halflings that was in Bootstrap Lite. Instead, I'm using the Unicode characters for left- and down-arrows. (However, there appears to be some issues with the rendering of fieldsets, which warrants a separate issue to address.)

This gets the overall size down to 8.2 MB, about half of which comes from the bundled minified Bootswatch themes.

bugfolder commented 1 year ago

Pinging @docwilmot: Are you OK to merge the PR (and do a new release)?

And related question: since the original PR, a newer version of the Bootstrap 5 library is out (5.3.1). Should I go ahead and upgrade the bundled version to that version in this PR?

docwilmot commented 1 year ago

And related question: since the original PR, a newer version of the Bootstrap 5 library is out (5.3.1). Should I go ahead and upgrade the bundled version to that version in this PR?

Sure. May as well make up to date before new release.

bugfolder commented 1 year ago

OK, Bundled version is now updated to 5.3.1 in the PR.

izmeez commented 1 year ago

I like the idea of providing an option to host the bootstrap bundle locally. I am wondering is it possible to have the bundle like an optional library so it is not embedded into the module itself? The bootstrap examples use a separate folder called "assets" with subfolders for "css" and "js". Would this structure lend itself to using libraries as a separate companion?

izmeez commented 1 year ago

Looking at this more I realize that I may be failing to understand the Backdrop approach. I see that https://docs.backdropcms.org/documentation/using-libraries suggests: `rather than creating one Backdrop module for the library and another for the Backdrop-specific functionality. When the library is bundled with the integration it provides, it ensures that the version of the library included will always work with the integration provided.

However, there are several cases where a separate library module would be preferred:`

bugfolder commented 1 year ago

I am wondering is it possible to have the bundle like an optional library so it is not embedded into the module itself?

The point of this issue was to bundle the library within the theme (and in this respect it mirrors what is done in the Bootstrap Lite theme).

As you noted (and other commenters above expressed), the Backdrop preferred methodology is to bundle libraries into the module or theme that uses them. The rationale for providing a separate module for the library would be if the library was likely to be be used by other modules but not this one. In this case, the Bootstrap Library is pretty tightly integrated into the theme, and it seems that most use cases would be subthemers who need it (not just the Bootstrap library itself, but its integration into Backdrop theming).

The main argument I could see for not bundling is to keep the file size down for developers using the CDN. In those cases, though, the developer could just drop the bundled library (and make sure they've selected the CDN, of course). Conversely, for those who want to use a bundled library rather than a CDN, this is the best way to provide it.

izmeez commented 1 year ago

The question of module size might be a factor, but if a CDN is selected the extra size of the module should not be adding a performance hit, unless I am missing something. The comments above seem to be suggesting it may be more maintenance for module updates and possibly throw off subthemes.

I was thinking of the case when a developer wants to get ahead with a newer bundle locally. As you say they could use the CDN or they could just swap out the bundle from the theme if they wished to. Yes, they would be out of sync with the module itself but it may not be an issue for a developer. They might also need to go backwards if needed for a subtheme.

bugfolder commented 1 year ago

I was thinking of the case when a developer wants to get ahead with a newer bundle locally.

Yes, that's now easy to do (with this change); just swap the newer bundle in place of the bootstrap folder in the theme, exactly as it's downloaded from Bootstrap.