Investigating Bootstrap4 with browse-everything

jrochkind commented 5 years ago

I have a new app that uses bootstrap 4, rather than bootstrap 3. Browse-everything assumes (and in some cases brings in as a dependency) bootstrap 3, creating certain challenges.

The good news is the actual func of 1.0.0.rc1 (which I am using) seems to work. The bad news is the BS3/4 conflict is real, and I'm not totally sure how b-e should handle this going forward, where some users will probably still be using BS3 and other BS4.

But here's my analysis of what's going on, which may be useful.

Different bootstrap gems

B-e actually has a gemspec dependency in it's gemspec on the bootstrap-sass gem.

bootstrap-sass is BS3 only. For BS4, there's a new gem, which is just called bootstrap. github

Will the BS3 one being a dependency alone cause a problem, if a given app additionally includes BS4 bootstrap? I'm not sure. At first I thought it didn't, and had my app basically working -- but now my app is not working, somehow the BS3 variables/mixins have infected my app as a whole even though at first they didn't.

Just having the gem as a dependency means sass (and js) files become part of the sprockets load path, so it may depend on your exact app which one(s) end up found by sprockets first if they are both present. It's not great.

B-E Generated SCSS file

The generated browse_everything.scss that becomes in in the local app includes a line @import 'bootstrap-sprockets';. bootstrap-sprockets doesn't exist in the bootstrap 4 gem, it's just @import bootstrap.

So initially this was erroring saying it couldn't find bootstrap-sprockets. (Actually not sure why it wasn't finding it in load path anyway from bootstrap-sass gem... but it would be bad if it were anyway, I don't want bootstrap 3 in my app).

This is easy enough to remove by editing the generated scss file in your local app.

Note also While not related to BS4, I don't understand this generated file in general.

My app is already including bootstrap in it's own CSS. The generated file having the import resulted in bootstrap CSS being included in sprockets compiled output twice, and I suspect many other apps are subject to this.
The generated file also has @import "font-awesome"; -- I can't figure out how/if font-awesome is really used at all by present B-E. Anyone know? Eliminating this didn't seem to harm B-E display at all. (The file/folder icons come from treetable, not font-awesome).

B-E CSS uses content-columns mixin.

B-E CSS uses the bootstrap3 content-columns mixin.

This mixin doesn't exist in BS4. So if you HAVE managed to get it in a BS4 environment, it will error there.

Experimentally, I made a local fork where I just removed that line from SCSS, to see how far I could get to working with that.

B-E CSS doesn't work right to create good display under BS4

This is unsurprisng, the CSS was written for BS3, and BS4 isn't entirely backwards compatible.

I wrote my own local CSS overrides to try and make it display right. This local CSS also does some things I just wanted locally -- like making the modal closer to "full screen" (I had done that customization even in my older sufia BS4 app. It may also "fix" some things I considered undesirable of weird-looking in "intended" B-E CSS.

Here is my current experimental local overrides

Modal mysteriously won't close

In my hacked together test app where I was seeing how far I could get like this, the B-E modal would not close. Even manually doing a $(element).modal("close"), it just wouldn't close.

If I removed the fade class from the modal, it worked again though. I tried to debug the bootstrap modal code, but other than confirming it was related to code specific to the fade transition, I couldn't really figure out what was going on.

In general, as far as I can tell, the way b-e is using modals is perfectly consistent with BS4 modal api, nothing has changed there.

It may be that somehow JS for bootstrap3 and bootstrap4 were getting confused together, or both loaded at once leading to dual event handling or something, due to the inclusion of bootstrap-sass gem and it effecting sprockets load paths.

What to do then?

I'm not quite sure what to do, for either b-e, or locally.

I could imagine a B-E release that somehow supported both BS3 and BS4. It would not include either gem as a dependency, the app would have to choose the dependency they wanted. (I think this is a good idea anyway, frankly). It could include two different CSS files, one for BS3 or one for BS4. It might include two different options for generated template SCSS -- but I'm not sure that generated template SCSS is a good idea anyway, I believe it was leading to double-inclusion of Bootstrap CSS in many real apps.

I could also imagine a B-E that supported just BS4. I'm not sure how workable that will be in the community though, inevitably the community is going to be split between people using BS3 and people using BS4 for a while -- unless instead everyone just stays stuck on BS3 forever. (Blacklight7, if it ever gets released, is BS4. It is very unclear to me the practical effects this will have on samvera ecosystem... it's gonna be painful, this is just the start).

It's unclear if these things can be done backwards compatibly, and how that effects release management, when we're already in the middle (but not the end) of a long process of trying to get a 1.0.

I can put at least some time into this, so if you (@mbklein ?) can make some decisions about what the heck we should actually do, I can probably contribute some code. It's very unclear to me what the "right" thing to do is.

Barring that, just thinking about what I can do to get my local app having B-E with bootstrap 4 -- I can't think of any alternative but making a BS4 fork of B-E. While hypothetically I could just use completely different CSS without a fork, I'm worried that B-E's dependency on bootstrap-sass, which infects sprocket load paths, is part of what's behind some of the mystery problems I'm having. And there's no way to get that but a fork. As much as I REALLY hate forks, I can think of no alternative here with current B-E -- other than not using B-E at all of course.

jrgriffiniii commented 5 years ago

I could also imagine a B-E that supported just BS4. I'm not sure how workable that will be in the community though, inevitably the community is going to be split between people using BS3 and people using BS4 for a while

Given the need for backward compatibility in so many Samvera Gems, I would fear that introducing this into a release would keep anyone from upgrading to the latest stable release.

jrochkind commented 5 years ago

In my current forked experiment, where I remove bootstrap-sass dependendency -- still can't get modal to close with "fade". Not sure what's going on there; maybe some weird incompatibility in Bootstrap 4 javascript with b-e javascript, but I can't find or see anything.

jrochkind commented 5 years ago

I share your fears, @jrgriffiniii , but note that Blacklight7 simply required bootstrap4, and did not support bootstrap3. But I honestly have no idea what the right way to proceed here is. I think it's going to be a problem across the eco-system.

jrgriffiniii commented 5 years ago

(Blacklight7, if it ever gets released, is BS4. It is very unclear to me the practical effects this will have on samvera ecosystem... it's gonna be painful, this is just the start)

This is true, but in a past discussion on a tech. call there explicitly stated some desire to have a Hyrax 3.x release integrate Blacklight 7. So Bootstrap 4 support will be required of this Gem, and this will likely warrant a second release for usage in Hyrax. I need to defer to others and investigate on my own whether or not this would absolutely require breaking changes.

jrochkind commented 5 years ago

At the very least, I think you gotta drop the bootstrap-sass dependency, which is technically arguably a backwards INcompat change (can't just upgrade without touching your app if you didn't include it already).

But yes, theoretically, I think a mostly (but not technically entirely) backwards-compat b-e could support bootstrap 3 or 4, with different CSS (and maybe but probably not different JS), both being made available.

If the Powers That Be think that's a good direction to work in, I can work on it and maybe PR some code. I am not sure how to deal with the timelines and git repo branches of b-e with the current in-progress 1.0.0.rc1, if it should be part of 1.0 or not (and it may have some minor app-setup-related backwards incompats).

Right now, I am experimentally just working on a fork of b-e that does bootstrap 4, just to see what i have to do, and maybe (regretfully) use it in my local app if I need it in my local app prior to anything being available upstream.

jrochkind commented 5 years ago

OK, I have a fork with b-e working on bootstrap4 (and even tests passing!).

As a bonus, it removed the explicit "sass-rails" dependency, so dependents can choose to use sassc-rails instead (sass-rails is "sunsetted", soon to be EOL; a dependency that depends on sass-rails makes it hard for a dependent to switch to sassc-rails).

Also removed font-awesome, because it did not seem to be used.

Please see the README for more info: https://github.com/samvera/browse-everything/compare/samvera:master...sciencehistory:bootstrap4

I think this could be turned into a PR with bootstrap4 support and bootstrap3 support.

I think it would be hard for it to be entirely backwards compat -- only in terms of integration, how you integrate with your app. A) Because it needs to not explicitly have a gem dependency on either bootstrap3 or 4 if it is going to work with both (different gems), B) That "intermediate" scss file was messy, I think it was not the best choice even with bootstrap 3 (pretty sure many apps were double-including all of bootstrap css as a result of double-import as a result of the current integration approach). And it was making it hard for me to get things to work right, especially considering paths for twin bootstrap 3+4 support, it complicated things enormously with no clear benefit.

So it probably would mean a new major version... which is really confusing when we're in process of 1.0.0.rc1 now. It could be part of a 1.0.0 (which will require some relatively easy changes to how it's integrated in apps compared to beta), or a 2.0.0 which comes out at about the same time as 1.0.0.

I have time and energy to spend more time on this and prepare actual PRs. But I need a product owner or core maintainer to work with me on it on the release management strategy and discussion of that change of approach.

Otherwise, i will just, regretfully, have my app depend on my fork. Which is there either way for people to look at for examples for any future work.

jrgriffiniii commented 5 years ago

Thank you very much for this work!

So it probably would mean a new major version... which is really confusing when we're in process of 1.0.0.rc1 now. It could be part of a 1.0.0 (which will require some relatively easy changes to how it's integrated in apps compared to beta)

I would say that merging this into a 1.0.0rc2 release is not at all unreasonable. Unless there are those who feel strongly about keeping further breaking changes from being merged into 1.0.0rc1, I would honestly prefer that this be addressed before a final 1.0.0 release. Otherwise I would agree that we would be risking breaking semantic versioning guidelines without looking to issue 2.0.0rc1 immediately.

mbklein commented 5 years ago

It's think it's actually great that this is getting in before 1.0.0, because the tweaks to backward compatibility are OK at this point. Definitely less confusing than a quick-turnaround 1.0/2.0 release.

jrochkind commented 5 years ago

OK, awesome, thanks for feedback! I'm going to make some PR's then, trying to separate em, and starting with the least controversial:

Remove font-awesome dependency, as far as we know it wasn't being used
Remove explicit sass-ruby dependency, so dependent apps can choose to use sass-ruby or sassc-ruby at their own convenience
Change the way that JS files are included (and remove explicit bootstrap3 dependency), so that apps can be more easily choose to use bootstrap3 or bootstrap4.

This last one will be the most controversial and "difficult" for existing adapters, as the way I have it working now, they will have to change the files that integrate B-E in their app. I am anticipating that the new integration instructions will be more like including @import browse-everything-bootstrap3 or @import browse-everything-bootstrap4 in your application.css -- with no generated intermediary file. I think making bootstrap3 or bootstrap4 explicit in the import is good to signal the change.

3 also has potentially other approaches that would work, but this is what I've figured out and have working smoothly.

jrochkind commented 5 years ago

There seems to be no "CHANGES" file. Should I add one, or otherwise make a place to keep track of notes on changes that might effect dependents?

samvera / browse-everything