Skip to content

Conversation

@jcoyne
Copy link
Member

@jcoyne jcoyne commented Oct 15, 2024

No description provided.

else
run "yarn add blacklight-frontend@#{Blacklight::VERSION}"
end
return generate_with_sassc_rails unless used_bootstrap_css?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, done.

@jrochkind
Copy link
Member

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.

@jcoyne jcoyne added this to the 9.X milestone Oct 16, 2024
@jrochkind
Copy link
Member

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 application.bootstrap.scss -- there are cases where you are using a package.json-based solution but didn't actually generate with --css=bootstrap, and you have an application.[s]css that you'd still want the @import 'blacklight' added to.

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?

@jcoyne
Copy link
Member Author

jcoyne commented Oct 17, 2024

Rebased

@jcoyne jcoyne merged commit da60832 into main Oct 17, 2024
13 checks passed
@jcoyne jcoyne deleted the sass_bootstrap branch October 17, 2024 18:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants