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

Implement FiltersPresenter#reset_url #3493

Merged
merged 1 commit into from
Oct 10, 2024
Merged

Implement FiltersPresenter#reset_url #3493

merged 1 commit into from
Oct 10, 2024

Conversation

csutter
Copy link
Contributor

@csutter csutter commented Oct 4, 2024

This returns the URL used for the "Clear all" link on both the filter
summary and filter panel in the new "all content" finder UI.

  • Add real implementation for FiltersPresenter#reset_url, merging the
    query parameters keys from every applied facet and then removing them
    from the current URL
  • Add UrlBuilder#url_except_keys to remove all given keys from the
    current finder URL's query parameters
  • Refactor duplication into a #applied_filters private method

Note this code could be simpler if we just removed all query params except the keyword, but then we would also lose any other query parameters that are unrelated to the filtering/sorting.

Review app testing

  • Go to a page with lots of applied filters
  • Try out both the "Clear all filters" links (next to "Apply filters" in the filter panel, and underneath the filter tags in the summary)

Copy link
Member

@kevindew kevindew left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍 Just a couple of minor comments.

Other thing is that given this is owned by #govuk-patterns-and-pages it doesn't feel like I should have the review without some team involvement. So imagine we want to check in with them.

app/presenters/filters_presenter.rb Outdated Show resolved Hide resolved
app/presenters/filters_presenter.rb Outdated Show resolved Hide resolved
app/presenters/filters_presenter.rb Outdated Show resolved Hide resolved
Copy link
Member

@kevindew kevindew left a comment

Choose a reason for hiding this comment

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

Nice simplification 👍

This returns the URL used for the "Clear all" link on both the filter
summary and filter panel in the new "all content" finder UI.

- Add real implementation for `FiltersPresenter#reset_url`, merging the
  query parameters keys from every applied facet and then removing them
  from the current URL
- Add `UrlBuilder#url_except_keys` to remove all given keys from the
  current finder URL's query parameters
- Refactor duplication into a `#applied_filters` private method
@govuk-ci govuk-ci temporarily deployed to finder-frontend-pr-3493 October 10, 2024 12:27 Inactive
@csutter csutter merged commit a0f018a into main Oct 10, 2024
12 checks passed
@csutter csutter deleted the clear-all branch October 10, 2024 12:31
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