Feature: Filter Aliases by Category/Type #1024

maxxcrawford · 2021-08-17T22:12:59Z

TODO:

Confirm mobile design/functionality
Add multiple filter support (Active, domain only)
Clicking off the pop-up should close it
Pressing "Esc" should close the pop-up
Add strings to FTL
~~Auto-apply the filtering when the checkbox is clicked (don't wait for the user to click "Apply")~~ Deferring this for now.

Bonus Items/issues:

Add keyboard controls
Create empty results state (Check with UX)
A11y check

Questions/Edge cases: (Answered by Eduardo on Aug 18)

~~What to do when two opposing (on/off) categories are selected?~~
- Answer: If one option conflicts with another, uncheck it. (psuedo radio buttons)
~~What happens when a user searches while the category filter is active?~~
- Answer: The filter only applies to active elements. If a user has category filtered, searching should only apply to those (and vice versa if search is active.
~~How do we show a user the category filter is active?~~
- Answer: For now, keep the filter icon hover state active.

maxxcrawford · 2021-08-19T15:57:58Z

static/scss/components/filter-bar.scss

@@ -10,7 +14,7 @@
        }
    }

-    &.is-search-active {
+    &.is-search-visible {
        margin-bottom: 4.5rem;


This is ~72px. The biggest the spacing- tokens go is 48px. Leaving as is.

maxxcrawford

Self JS review

maxxcrawford · 2021-08-19T15:59:01Z

static/js/filter.js

+    let currentFilteredByCategoryAliases;
+    let currentFilteredBySearchAliases;


let sniff: This is part of some wild west state management. If we want to flag it for a state. type object, we can refactor heavily to manage this.

Ugh, some random state object flying around would be worse than let anyway. If I can't come up with a refactor in 10-15m I'm fine with keeping let

It's worth exploring later on, when we have even more search going on. When filters get stacked on top of each other, having single point of reference of what should be filtered is helpful.

static/js/filter.js

maxxcrawford · 2021-08-19T15:59:51Z

static/js/filter.js

    function toggleAliasSearchBar() {
        filterToggleSearchInput.classList.toggle("is-enabled");
-        filterContainer.classList.toggle("is-search-active")       
+        filterContainer.classList.toggle("is-search-visible");


This class is used to toggle (via CSS) search on mobile/tablet.

static/js/filter.js

maxxcrawford · 2021-08-19T16:02:00Z

static/js/filter.js

+        if (isCategoryFilterActive) {
+            // aliases = 
+            currentFilteredByCategoryAliases = document.querySelectorAll(".c-alias:not(.is-hidden)");
+        } else {
+            filterContainer.classList.add("is-filtered-by-search");
+        }


This logic block appears throughout this PR and is the crux of the overall feature. Basically when either search or category filtering is active, we have to trunicate the list of available aliases for the OTHER filter to query from. Moving to state management would drop this logic, giving each filtering type the same centralized "target" to query from.

Yeah this feels like we're getting closer and closer to doing a full-on Redux/React refactor sometime.

static/js/filter.js

maxxcrawford · 2021-08-19T16:05:02Z

static/js/filter.js

+        oppositeCheck: (e) => {
+            const currentCategory = e.target;
+
+            if (!currentCategory.checked) {
+                return;
+            }
+
+            const currentParentCategory = currentCategory.dataset.parentCategory;
+
+            filterCategoryCheckboxes.forEach(checkbox => {
+                if ( (currentCategory !== checkbox) && (checkbox.dataset.parentCategory === currentParentCategory) && checkbox.checked) {
+                    checkbox.checked = !checkbox.checked;
+                }
+            });


This an edgecase function to make the checkboxes from the UI behave like radio buttons in certain cases:

Example: User opens filter and selects "show all active aliases" AND "show all disabled aliases". Since this would make the filter…useless, we detect this and toggle off the opposing checkbox.

groovecoder

I don't see any cardinal sins in this code. There's a couple UX behaviors we might want to do?

Clicking off the pop-up should close it
Pressing "Esc" should close the pop-up
Auto-apply the filtering when the checkbox is clicked (don't wait for the user to click "Apply")

privaterelay/templates/includes/dashboard-filter.html

groovecoder · 2021-08-20T19:50:04Z

privaterelay/templates/includes/dashboard-filter.html

+                                id="active-aliases"
+                                value=""
+                            >
+                            <label class="mzp-c-choice-label" for="active-aliases">Active aliases</label>


The English strings need to be internationalized ... unless these aren't final UI strings, in which case, ignore.

Strings added.

groovecoder · 2021-08-20T19:52:04Z

static/js/filter.js

+    let currentFilteredByCategoryAliases;
+    let currentFilteredBySearchAliases;


Ugh, some random state object flying around would be worse than let anyway. If I can't come up with a refactor in 10-15m I'm fine with keeping let

groovecoder · 2021-08-20T19:53:38Z

static/js/filter.js

+        if (isCategoryFilterActive) {
+            // aliases = 
+            currentFilteredByCategoryAliases = document.querySelectorAll(".c-alias:not(.is-hidden)");
+        } else {
+            filterContainer.classList.add("is-filtered-by-search");
+        }


Yeah this feels like we're getting closer and closer to doing a full-on Redux/React refactor sometime.

static/js/filter.js

static/scss/app.scss

… class.

…results to reset category filter

…t category type

…a search

…lter is user clicks outside of category filter

codemist

Looks good to me, just nits :)

privaterelay/templates/includes/dashboard-filter.html

maxxcrawford · 2021-08-25T17:04:39Z

Related PR for strings: mozilla-l10n/fx-private-relay-l10n#5

maxxcrawford added the 🚧 WIP Work in Progress label Aug 17, 2021

maxxcrawford linked an issue Aug 17, 2021 that may be closed by this pull request

Add Alias Filter Feature: Filter by Attribute/Category #1023

Closed

maxxcrawford force-pushed the 1023-filter-category branch 2 times, most recently from 4b3d643 to 723d50e Compare August 19, 2021 15:47

maxxcrawford requested a review from groovecoder August 19, 2021 15:48

maxxcrawford marked this pull request as ready for review August 19, 2021 15:56

maxxcrawford requested a review from codemist August 19, 2021 15:56

maxxcrawford commented Aug 19, 2021

View reviewed changes

maxxcrawford removed the 🚧 WIP Work in Progress label Aug 20, 2021

groovecoder reviewed Aug 20, 2021

View reviewed changes

maxxcrawford force-pushed the 1023-filter-category branch from 723d50e to 368005d Compare August 23, 2021 21:50

maxxcrawford added 14 commits August 25, 2021 09:54

Unhide filter button

e8822a3

Remove drop-shadow from deactivated alias, remove unused card-enabled…

ce00309

… class.

Fix #1023 - Add category filter UI

d0a00b4

Add JavaScript logic to show/hide category filter

fd34b13

Add checkbox logic to begin filter function

4494412

Add filter logic, custom data class names to aliases for targeting

d723bdf

Add feature where multiple filters can be applied at once

d083559

Fix issue where only category filter items visible from search query

b04b607

Make category filter active while aliases are filtered, cache search …

b2af7e0

…results to reset category filter

Edgecase: Add category logic to only select one filter for each paren…

d54334d

…t category type

Feature: Add ability to filter by category and then refine results vi…

cf6240b

…a search

Add mobile styles

44ae01b

Lint cleanup/whitespace fixes

ac52657

Add esc key listener to close category filter when open, and close fi…

10de4be

…lter is user clicks outside of category filter

maxxcrawford force-pushed the 1023-filter-category branch from d8416ca to 10de4be Compare August 25, 2021 14:56

Add labels to category buttons, match figma still more

efb789d

maxxcrawford requested a review from groovecoder August 25, 2021 15:53

Add strings to FTL, html whitespace

6002467

maxxcrawford force-pushed the 1023-filter-category branch from 0eb794c to 6002467 Compare August 25, 2021 16:00

codemist approved these changes Aug 25, 2021

View reviewed changes

privaterelay/templates/includes/dashboard-filter.html Outdated Show resolved Hide resolved

privaterelay/templates/includes/dashboard-filter.html Outdated Show resolved Hide resolved

maxxcrawford added 2 commits August 25, 2021 11:38

Lint SCSS fix

f4b9d4a

Whitespace HTML fix

2990194

maxxcrawford mentioned this pull request Aug 25, 2021

New strings for fx-private-relay/pull/1024 mozilla-l10n/fx-private-relay-l10n#5

Merged

groovecoder approved these changes Aug 25, 2021

View reviewed changes

maxxcrawford merged commit 34e94f3 into main Aug 25, 2021

groovecoder deleted the 1023-filter-category branch October 7, 2021 18:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature: Filter Aliases by Category/Type #1024

Feature: Filter Aliases by Category/Type #1024

maxxcrawford commented Aug 17, 2021 •

edited

Loading

maxxcrawford Aug 19, 2021

maxxcrawford left a comment

maxxcrawford Aug 19, 2021

groovecoder Aug 20, 2021

maxxcrawford Aug 23, 2021

maxxcrawford Aug 19, 2021

maxxcrawford Aug 19, 2021

groovecoder Aug 20, 2021

maxxcrawford Aug 19, 2021

groovecoder left a comment

groovecoder Aug 20, 2021

maxxcrawford Aug 25, 2021

groovecoder Aug 20, 2021

groovecoder Aug 20, 2021

codemist left a comment •

edited

Loading

maxxcrawford commented Aug 25, 2021

		let currentFilteredByCategoryAliases;
		let currentFilteredBySearchAliases;

Feature: Filter Aliases by Category/Type #1024

Feature: Filter Aliases by Category/Type #1024

Conversation

maxxcrawford commented Aug 17, 2021 • edited Loading

Choose a reason for hiding this comment

maxxcrawford left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

groovecoder left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codemist left a comment • edited Loading

Choose a reason for hiding this comment

maxxcrawford commented Aug 25, 2021

maxxcrawford commented Aug 17, 2021 •

edited

Loading

codemist left a comment •

edited

Loading