Skip to content

Conversation

jazairi
Copy link
Contributor

@jazairi jazairi commented Apr 5, 2024

Why these changes are being introduced:

There is not currently an affordance to remove all currently applied filters.

Relevant ticket(s):

How this addresses that need:

This adds the requested button, which uses a new remove_all_filters helper method that generates a new query in a similar fashion to the add_filter and remove_filter helpers.

Side effects of this change:

The applied_filters method now takes a query argument rather than accessing the @enhanced_query within the method. This felt like good practice and more consistent with how we structured our other filter helper methods (including the newly added one).

Developer

Accessibility
  • ANDI or WAVE has been run in accordance to our guide.
  • This PR contains no changes to the view layer.
  • New issues flagged by ANDI or WAVE have been resolved.
  • New issues flagged by ANDI or WAVE have been ticketed (link in the Pull Request details above).
  • No new accessibility issues have been flagged.
New ENV
  • All new ENV is documented in README.
  • All new ENV has been added to Heroku Pipeline, Staging and Prod.
  • ENV has not changed.
Approval beyond code review
  • UXWS/stakeholder approval has been confirmed.
  • UXWS/stakeholder review will be completed retroactively. (Darcy has approved the design, but she hasn't tested it live.)
  • UXWS/stakeholder review is not needed.
Additional context needed to review

While the diff makes it look like there are a lot of SCSS changes, most of it is an extra indentation. All of the rules from line 108-127 of _panels.scss are now on a .list-filter-summary selector. This basically bumps the flex container down one level in the DOM, so the new button can inherit the .filter-summary rules without inheriting display: flex.

In terms of confirming functionality, the button should appear in the search summary panel if two or more filters are applied. When clicked, it should remove all filters and reset the page number to 1; all other query terms should remain unaffected.

Code Reviewer

Code
  • I have confirmed that the code works as intended.
  • Any CodeClimate issues have been fixed or confirmed as
    added technical debt.
Documentation
  • The commit message is clear and follows our guidelines
    (not just this pull request message).
  • The documentation has been updated or is unnecessary.
  • New dependencies are appropriate or there were no changes.
Testing
  • There are appropriate tests covering any new functionality.
  • No additional test coverage is required.

@mitlib mitlib temporarily deployed to timdex-ui-pi-gdt-223-cl-spgwwu April 5, 2024 20:22 Inactive
@coveralls
Copy link

coveralls commented Apr 5, 2024

Pull Request Test Coverage Report for Build 8632614985

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 8 of 8 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.006%) to 99.435%

Totals Coverage Status
Change from base Build 8571510856: 0.006%
Covered Lines: 528
Relevant Lines: 531

💛 - Coveralls

@jazairi jazairi force-pushed the gdt-223-clear-all-filters branch from c22440a to e85f1a2 Compare April 8, 2024 13:21
@JPrevost JPrevost self-assigned this Apr 8, 2024
@mitlib mitlib temporarily deployed to timdex-ui-pi-gdt-223-cl-eoy6rb April 9, 2024 14:36 Inactive
Copy link
Member

@JPrevost JPrevost left a comment

Choose a reason for hiding this comment

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

Approved but with a suggestion for some additional tests we may find useful to have in place. If you agree, we can add them. If you don't, you can merge as-is.

assert_not filter_applied?(query[:contentTypeFilter], 'dataset')
end

test 'applied_filters returns all currently applied filters' do
Copy link
Member

Choose a reason for hiding this comment

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

Thoughts on a controller test confirming the button does not show up (or at least isn't visible) unless the expected conditions are met? I would be okay with not including it, but I also think it may be helpful to confirm at that level.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that makes sense and should be easily testable.

@jazairi jazairi temporarily deployed to timdex-ui-pi-gdt-223-cl-eoy6rb April 9, 2024 18:46 Inactive
@jazairi jazairi requested a review from JPrevost April 9, 2024 18:53
Why these changes are being introduced:

There is not currently an affordance to remove all currently
applied filters.

Relevant ticket(s):

* [GDT-223](https://mitlibraries.atlassian.net/browse/GDT-223)

How this addresses that need:

This adds the requested button, which uses a new `remove_all_filters`
helper method that generates a new query in a similar fashion to
the `add_filter` and `remove_filter` helpers.

Side effects of this change:

* The `applied_filters` method now takes a `query` argument rather
than accessing the `@enhanced_query` within the method. This felt
like good practice and more consistent with how we structured our
other filter helper methods (including the newly added one).
* Two cassettes have been added to accommodate new tests.
@jazairi jazairi force-pushed the gdt-223-clear-all-filters branch from 850c78b to 54946d7 Compare April 10, 2024 14:03
@jazairi jazairi temporarily deployed to timdex-ui-pi-gdt-223-cl-eoy6rb April 10, 2024 14:03 Inactive
@jazairi jazairi merged commit 4a46796 into main Apr 10, 2024
@jazairi jazairi deleted the gdt-223-clear-all-filters branch April 10, 2024 14:05
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.

4 participants