Don't install the bootstrap gem unless they are using sassc #3363
Conversation
76b37eb to
bd4a043
Compare
| else | ||
| run "yarn add blacklight-frontend@#{Blacklight::VERSION}" | ||
| end | ||
| return generate_with_sassc_rails unless used_bootstrap_css? |
There was a problem hiding this comment.
I would find this more readable if it were
unless used_bootstrap_css?
generate_with_sassc_rails
return
(Unless you actually want to use the return value of generate_with_sassc_rails).
This code is getting pretty convoluted and branching, keeping it readable seems good, what do you think?
|
This does not fix the test-order-dependent Rails 8 failure in #3361, but i guess it was not intended to? The logic does seem right. The logic is getting pretty convoluted and hard to follow, and kind of starts to seem "accidental" to me; I think it could use refactoring these asset generators into more clear use cases, instead of just making tests green. I could be interested in submitting a PR for such a refactor, but resolving the test-order-dependent failures in Rails 8 seems like a pre-requisite, to be confident green on such a PR was actually a signal the refactor works out. |
bd4a043 to
41f915e
Compare
|
I think this makes sense (and is similar to what I ended up doing in a version in blacklight_range_limit investigating this stuff). I am interested in if it breaks #3360 if we merge that first. At some point I'd be interested in adding a switch so the file does not NEED to be named But that's a separate thing, this PR doesn't change it. I'll approve, but what if we merge #3360 first and rebase this one to make sure they don't conflict? |
41f915e to
e436d71
Compare
|
Rebased |
No description provided.