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

Simply documented and tested config to activate range limit facet #275

Merged
merged 3 commits into from
Nov 13, 2024

Conversation

jrochkind
Copy link
Member

@jrochkind jrochkind commented Nov 12, 2024

In the old days, you activated a range limit config with eg

config.add_facet_field 'pub_date_si', label: 'Publication Date Sort', range: true

At some time that was changed to use a default_range_config method mixed into CatalogController...

config.add_facet_field 'pub_date_si', label: 'Publication Date Sort', **default_range_config

The former simpler way of doing it now works again. And you can override other default config simply by adding the config you want to override:

config.add_facet_field 'pub_date_si', label: 'Publication Date Sort', range: true, range_config: { textual_facets: false }

Changed docs and tests to go back to simpler original way. Also will include specific docs for over-riding Components.

For now, the mixed-in default_range_config method is still available in CatalogControllers.. we could remove it, it would be backwards incompat with any existing config, but it is a major version, any thoughts? I'd just leave it in for now prob... maybe deprecate?

@jrochkind
Copy link
Member Author

not sure why CI isn't running, but I built on top of #274 to avoid conflict, so will rebase and make Ready when that one is resolved.

Base automatically changed from collapsible_textual_facets to main November 12, 2024 21:33
@jrochkind jrochkind marked this pull request as ready for review November 12, 2024 21:41
@jrochkind jrochkind marked this pull request as draft November 12, 2024 21:42
@jrochkind jrochkind marked this pull request as ready for review November 12, 2024 21:45
@seanaery
Copy link
Collaborator

This looks like a good change to me, making this syntax what's recommended in the README. Nice to see that works again, as it feels cleaner/simpler. No strong opinion on whether to drop, keep, or deprecate support for **default_range_config.

@seanaery seanaery merged commit 9ba9922 into main Nov 13, 2024
9 checks passed
@seanaery seanaery deleted the fix_config_suggestion branch November 13, 2024 19:11
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.

2 participants