Skip to content
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

Convert range limit views + helpers to presenters + components #179

Merged
merged 16 commits into from
May 5, 2022

Conversation

cbeer
Copy link
Member

@cbeer cbeer commented Dec 11, 2021

Pending a release with projectblacklight/blacklight#2594

This PR is perhaps not strictly backwards compatible for applications with extensive customizations, but is functionally equivalent and the public field configuration remains the same. Additionally, I've preserved the helper methods that were actively used by the old partials, so this should also be non-disruptive for applications that customized the partials.

With that, and in the interest of keeping the major version number of this project similar to the blacklight major version number, I think there's a reasonable argument for just bumping the minor version in a release.

Upgrade guide

If you have no local customizations for the range limit plugin, there's no additional steps required.

The most common local customization seems to be overrides to combine multiple blacklight plugins that override the same upstream methods.

This new approach should not require those overrides and the components and presenters should work for you as-provided. Make sure to remove any local blacklight_range_limit/_range_segments.html.erb view partial and consider updating your facet field configuration to the new recommended approach:

config.add_facet_field 'pub_date', label: 'Publication Year', **default_range_config

Finally, if you have additional local customizations, these should be adapted into local presenters + components that extend and override the provided classes as appropriate. In general:

  • overrides to the display of the selected range in constraints (likely by overriding the range_display helper) can be done with a custom FacetItemPresenter
  • overrides that control how range limit URL parameters are constructed can be done in a custom FilterField
  • overrides that change how solr response information is mapped into blacklight can be done with a custom FacetFieldPresenter

Note: Some CSS classes and URL parameters have changed to match those provided by blacklight for ordinary facet fields.

@cbeer cbeer marked this pull request as draft December 11, 2021 00:51
@cbeer cbeer force-pushed the view-components branch 2 times, most recently from 4c80fa3 to a78c69f Compare December 20, 2021 17:11
Copy link
Contributor

@barmintor barmintor left a comment

Choose a reason for hiding this comment

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

Some minor performance and testing possibilities, but nothing major. I think there's something to be gained from minting a major rev here and moving on to bugfixes - this is generally the approach I also used. I would also like to open a related PR to the Blacklight 7.x branch to rely more on filters than direct inspection of params[:f] so that some of the deprecated methods would still work against this, but we can discuss separately.

lib/blacklight_range_limit/range_limit_builder.rb Outdated Show resolved Hide resolved
spec/features/blacklight_range_limit_spec.rb Show resolved Hide resolved
spec/features/blacklight_range_limit_spec.rb Outdated Show resolved Hide resolved
@barmintor
Copy link
Contributor

Hmm failing on net/smtp w/ Ruby 3

@jrochkind
Copy link
Member

jrochkind commented May 5, 2022

net/smtp moves from being a "default gem" to a "bundled gem" in ruby 3.0.

https://stdgems.org/

"The behavior of bundled gems is similar to normal gems, but they get automatically installed when you install Ruby. They can be uninstalled and they are maintained outside of Ruby core."

What you are supposed to do about this in software that uses it as a dependency and is intended to work on multiple ruby versions.... continues to really confuse me. I guess add it to your gem dependencies? Whether this can mess things up when running on older versions of ruby I don't understand though?

There is a long Issue on this... somewhere... not I can't find it, that at one point I tried to participate in, and I ended up just more really confused. Not ruby's strongest hour.

@jrochkind
Copy link
Member

Here's some of the long and confusing trail of people trying to deal with this. I think mail is in fact the dependency that's using net/smtp, relevant to us? mikel/mail#1439

Which led to this...

mikel/mail#1472

Looks like mail may finally be about to solve the problem, and has a 2.8.0.rc1 release that does? But as an rc release, it won't be used here. It may be the problem gets solved whenever mail 2.8.0 gets released.

If ruby 3.0 inclusion in CI was not added in this PR but was pre-existing, I'm not sure why the problem is showing up only now. Very confusing.

@barmintor
Copy link
Contributor

@jrochkind it's fixed with a Rails bump in the matrix; I've rebased to use a similar matrix to Blacklight.

@jrochkind
Copy link
Member

jrochkind commented May 5, 2022

oh sweet, thanks for the info @barmintor. The matrix is locked to exact Rails patch version I guess? OK!

I don't see any change to circleci config in this PR though, so I'm confused how you did that! I'm was curious to see what specific Rails version change did it.

@barmintor barmintor marked this pull request as ready for review May 5, 2022 18:47
@barmintor barmintor merged commit 4795ac1 into master May 5, 2022
@cbeer cbeer deleted the view-components branch June 3, 2022 15:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants