Fix filter display wrapping and clean up tests #319

sarayourfriend · 2021-10-14T17:13:48Z

Fixes

Description

Fixes the active filter display by making them flex wrap. Also converts it to tailwind for the most part except for the button and tiny classnames which I assume we'll get rid of when we do the full redesign anyway.

Also removes all the existing tests for the FilterDisplay component in favor of a single snapshot tests.

I don't love snapshot tests, but in this case the tests that existed there previously either (a) didn't actually do anything (in particular the "checked" test was completely useless) or (b) were manual snapshot tests, directly testing the markup. So I just replaced it with a single snapshot test that at least verifies that the component renders as expected and requires updating if anything changes.

It's probably, ultimately, a fragile snapshot test, but I'd personally like to see this component refactored in a number of ways to make it more testable and simpler. For one, we could split it into a container and display component. The container would connect to the store and the display would taking things as normal props. It would massively simplify the tests against the display component because we could just use normal props instead of having to mock out a whole store and getters and stuff.

In any case, that's a project for another day. I think the current set up makes the most sense from a perspective of (a) preserving what testing was already being done while (b) removing tests that weren't actually doing anything.

Testing Instructions

Checkout the branch and run npm run dev. Then perform a search and activate enough filters to cause the active filters list to wrap and verify that it looks okay.

Checklist

My pull request has a descriptive title (not a vague title like Update index.md).
My pull request targets the default branch of the repository (main) or a parent feature branch.
My commit messages follow best practices.
My code follows the established code style of the repository.
I added or updated tests for the changes I made (if applicable).
I added or updated documentation (if applicable).
I tried running the project locally and verified that there are no visible errors.

Developer Certificate of Origin

Developer Certificate of Origin
Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
1 Letterman Drive
Suite D4700
San Francisco, CA, 94129

Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.


Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.

obulat · 2021-10-15T12:51:27Z

src/components/Filters/FilterDisplay.vue

-      $t('filters.filter-by')
-    }}</span>
+  <div class="flex flex-wrap items-center p-4" aria-live="polite">
+    <span v-if="isAnyFilterApplied" class="mr-2 font-semibold">


Suggested change

<span v-if="isAnyFilterApplied" class="mr-2 font-semibold">

<span v-if="isAnyFilterApplied" class="mr-2 mb-2 font-semibold">

mb-2 makes the 'Filter by' span aligned with the tags.

obulat · 2021-10-15T12:53:37Z

src/components/Filters/FilterTag.vue

@@ -1,6 +1,6 @@
 <template>
  <button
-    class="filter-block button tiny tag mx-1"
+    class="filter-block button tiny tag mx-1 mb-2"


It might be better to move the margin declarations to the top: <FilterTag class="mx-1 mb-2 /> so that the layout is handled by the parent component.

obulat

Looks and works great! I've added a couple of non-blocking comments inline.

sarayourfriend · 2021-10-15T13:27:29Z

@obulat Thanks for the review. I made the changes you suggested... and then went down a rabbit hole to fix the FilterTag tests as well and fix some accessibility issues in that component, so this could do with a re-review from you if you have time 🙂

obulat · 2021-10-15T15:02:51Z

src/components/Filters/FilterTag.vue

@@ -1,17 +1,18 @@
 <template>
-  <button
-    class="filter-block button tiny tag mx-1"
+  <div


I think it would be better for user experience to keep it a button, and add the onClick handlers for button as a whole, not just the cross span. I've tried to look for examples of dismissible tags, and only Walmart came to mind: their tags can be dismissed by clicking anywhere in the button, not just the cross icon. It might also be bad user experience for people with big hands on mobiles who cannot click on such a small target.

By the way, I've also tried removing the @keyup.enter handler from the button, leaving only the @click one, and it still works, because the browser understand Enter or Space presses while the focus is on the button as button clicks. But I'm still not 100% sure it's a good idea to remove the Enter handling ...

Okay! That makes sense to me. I changed it because the click handler was only on the span, which wasn't accessibile. But if we make the whole thing a dismiss button then that fixes it. Also easier to click the whole thing rather than just the cross.

hmmm, one wrinkle... I'm not sure how to communicate to screen readers that the button is a close button. I'll have to do some digging.

Okay in 7d8952b I updated it so the whole tag is a close button with an accessible label.

VoiceOver on macOS and NVDA both only read the aria label on a button when it exists, so we need to include the type of filter in the label or else the screen reader just reads "Remove filter" with no extra context about which filter would be removed.

obulat

Thank you for updating the tests! I am also uneasy about using snapshot tests, because they've previously broke so many times for me, and failed me even when the linters updated the .snap line spacing...

sarayourfriend · 2021-10-15T15:17:34Z

Thank you for updating the tests! I am also uneasy about using snapshot tests, because they've previously broke so many times for me, and failed me even when the linters updated the .snap line spacing...

Yeah... I really only used them because the existing tests were just as brittle and required manual updating and were testing irrelevant implementation details... so somehow even snapshot tests are better I think 😂

sarayourfriend added 🛠 goal: fix Bug fix 🕹 aspect: interface Concerns end-users' experience with the software labels Oct 14, 2021

sarayourfriend requested a review from a team as a code owner October 14, 2021 17:13

sarayourfriend requested review from AetherUnbound and dhruvkb October 14, 2021 17:13

dhruvkb added this to Needs review in Openverse PRs Oct 14, 2021

obulat reviewed Oct 15, 2021

View reviewed changes

obulat approved these changes Oct 15, 2021

View reviewed changes

sarayourfriend requested a review from obulat October 15, 2021 13:27

obulat reviewed Oct 15, 2021

View reviewed changes

obulat approved these changes Oct 15, 2021

View reviewed changes

sarayourfriend added 5 commits October 15, 2021 10:05

Fix filter display wrapping and clean up tests

d84b7f5

Remove custom classname

d364a12

Reorganize classes and align label with filter tags

f9cf716

Reorganize classes, align label with filter tags, fix filtertag tests

b8fecfb

Use more accessible remove filter button label

7d8952b

sarayourfriend force-pushed the fix/active-filter-wrapping branch from 352fa6b to 7d8952b Compare October 15, 2021 17:06

Openverse PRs automation moved this from Needs review to Reviewer approved Oct 16, 2021

zackkrida approved these changes Oct 16, 2021

View reviewed changes

sarayourfriend merged commit 1e0a948 into main Oct 16, 2021

Openverse PRs automation moved this from Reviewer approved to Merged! Oct 16, 2021

sarayourfriend deleted the fix/active-filter-wrapping branch October 16, 2021 18:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix filter display wrapping and clean up tests #319

Fix filter display wrapping and clean up tests #319

sarayourfriend commented Oct 14, 2021

obulat Oct 15, 2021

obulat Oct 15, 2021

obulat Oct 15, 2021

obulat left a comment

sarayourfriend commented Oct 15, 2021

obulat Oct 15, 2021

sarayourfriend Oct 15, 2021

sarayourfriend Oct 15, 2021

sarayourfriend Oct 15, 2021

obulat left a comment

sarayourfriend commented Oct 15, 2021

	<span v-if="isAnyFilterApplied" class="mr-2 font-semibold">
	<span v-if="isAnyFilterApplied" class="mr-2 mb-2 font-semibold">

Fix filter display wrapping and clean up tests #319

Fix filter display wrapping and clean up tests #319

Conversation

sarayourfriend commented Oct 14, 2021

Fixes

Description

Testing Instructions

Checklist

Developer Certificate of Origin

obulat Oct 15, 2021

Choose a reason for hiding this comment

obulat Oct 15, 2021

Choose a reason for hiding this comment

obulat Oct 15, 2021

Choose a reason for hiding this comment

obulat left a comment

Choose a reason for hiding this comment

sarayourfriend commented Oct 15, 2021

obulat Oct 15, 2021

Choose a reason for hiding this comment

sarayourfriend Oct 15, 2021

Choose a reason for hiding this comment

sarayourfriend Oct 15, 2021

Choose a reason for hiding this comment

sarayourfriend Oct 15, 2021

Choose a reason for hiding this comment

obulat left a comment

Choose a reason for hiding this comment

sarayourfriend commented Oct 15, 2021