-
Notifications
You must be signed in to change notification settings - Fork 64
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
Adjust versions and arguments so tests pass #3181
Conversation
Somewhere we lost the jQuery integration for BS4 (e.g., modal, tab) in some contexts (e.g., zpr_links.js) when importing BS4 in the app's application.js to be bundled with the default esbuild. It seems to work when included in the layout. Let's see if this passes the test suite.
43fb24b
to
a892c51
Compare
@taylor-steve Can these changes be split into separate PRs? These three changes seem like they could stand on their own. I don't see how the Dockerfile changes relate to the tests passing. |
@jcoyne This is for the propshaft branch and those Dockerfile changes were needed to make the docker build check pass. The team is still debating how to handle rewriting history on the overall branch. |
@taylor-steve can the changes not relating to propshaft (like .ruby.yml) be made as separate PRs on main? Then you can merge those commits into the propshaft branch once they are accepted. This will cause the least departure from the main branch and make it much easier for people to review the propshaft changes. |
Can you explain
Importmap:install will happen well before application.js is written, right? |
Yes, that makes sense.
The behavior I see is that |
@taylor-steve how about using |
They are unnecessary. The Blacklight frontend package is based on the actual installed gem version. After the switch to using '^', the highest allowed version of bootstrap will be installed anyway.
This ensures the importmap generator runs before any of the spotlight generators
The goal of this PR is to get the tests passing, so we have a real baseline. This way we can get CI feedback if we break things.
Switches from
~
to^
for bootstrap.yarn build:css
/rake assets:precompile
was failing in some cases because with the current setup the propshaft generator was literally installing bootstrap 4.0.Has a temporary (?) fallback mechanism so that in cases like BL 7.39.0 that do not have a corresponding NPM package it will try and install the latest version for that major release.
[Temporary Workaround] Somewhere along the way we (probably me!) broke something related to Bootstrap 4 and bundling. When imported in application.js and bundled via esbuild, we were losing functions like
modal
in files likezpr_links.js
. Long story short our lives would be much easier if we can drop eitherjquery-serializejson
or Bootstrap 4, but I will attempt to review the history and find out where the bundling broke when I return. For now, as a stopgap, I'm including the Bootstrap 4 bundle package in the layout when BS4 is installed.