DOP-3690: Initial implementation of Search Facets UI #925

rayangler · 2023-09-29T14:29:53Z

Stories/Links:

Current Behavior:

Staging Links:

Search page with feature flag - no facets applied
Search page with feature flag - facets applied
Search page without feature flag - old filters, should have no changes from prod

Notes:

The order and names of the facet options/categories may not currently match the designs. This is due to the current order of the taxonomy. The frontend should be responsible for rendering the order of the data as-is. A separate change may be needed to update the taxonomy accordingly.
Versions are not currently included in this UI due to lack of data. These can be added separately.
Selecting facets currently does NOT update the search results. I believe this may intersect with DOP-3689's goal of having the context responsible for API requests.

mmeigs

Looks gorgeous. Two things:

Should there be downward arrows to the left of these option groupings that have hidden expansions? This could be a later ticket, just wondering. I'll attach an example from the figma.
Note: it's a bit confusing because in the figma all the examples are for versions (which are ignored here, I know), but I'm wondering if options such as "Atlas" which have so many other options nested should have the arrows and option to open it up without selecting it.

TOTAL nit. But I had a slow brain moment for a while looking back and forth between FacetOption and FacetValue. Thinking there might be a better naming convention? I do not have perfect "options" here but something like FacetOption -> FacetGroup(ing) and FacetValue -> FacetOption? Not sure. Also, disregard if you like!!

mmeigs · 2023-09-29T17:48:56Z

src/components/SearchResults/Facets/FacetOption.js

+  font-size: 13px;
+  line-height: 20px;
+  color: ${palette.gray.dark4};
+  margin: 8px 0;


nit: In some places, this feels a little squished on the bottom compared to the Figma file.

Done! Changed the bottom to be 16px, as recommended by Iz

mmeigs · 2023-09-29T17:52:55Z

src/components/SearchResults/Facets/FacetOption.js

+      {truncated && (
+        <div className={cx(showMoreStyle)} onClick={handleExpandResults}>
+          <Icon className={cx(showMoreGlyphStyle)} glyph={'ChevronDown'} />
+          Show more
+        </div>
+      )}
+    </div>
+  );


I think it's worth asking Iz if we want a "show less" option as well for the opposite of these.

You're right! Done!

mmeigs · 2023-09-29T18:07:09Z

src/components/SearchResults/SearchContext.js

+    ({ target, key, id }) => {
+      const { id: fullFacetId, checked } = target;
+
+      // Update query params based on whether a facet a is being added or removed


Just typing error :)
"on whether a facet is being"

Hehe, good catch. I think I ended up removing the comment as part of a small refactor :P

mmeigs · 2023-09-29T18:12:44Z

src/components/SearchResults/Facets/FacetOption.js

+        <Body className={cx(optionNameStyle)} weight={'bold'}>
+          {name}
+        </Body>


This is random, and I don't want to hardcode, but are we going with "Target Product" as the name for "Product" or do we need to change the data model or do we need to create a frontend translation?

we will have to change the taxonomy file for preservation of order and naming

seungpark

this looks great! couple of minor design comments and code cleanup comments below. thanks for putting the front end to fruition @rayangler !

src/components/SearchResults/SearchResults.js

src/components/SearchResults/SearchContext.js

src/components/SearchResults/Facets/useFacets.js

src/components/SearchResults/Facets/FacetValue.js

seungpark · 2023-10-02T14:04:04Z

src/components/SearchResults/Facets/FacetValue.js

+      />
+      {isChecked &&
+        facets.map((facet) => {
+          return <FacetOption key={facet.id} className={cx(nestedFacetStyle)} facetOption={facet} isNested={true} />;


should we just pass the isNested prop? seems like we can style here for FacetOption but we are passing a classname for FacetOption styling set in FacetValue

Done. Moved the styling accordingly

seungpark · 2023-10-02T14:04:54Z

src/components/SearchResults/Facets/FacetOption.js

+        <Body className={cx(optionNameStyle)} weight={'bold'}>
+          {name}
+        </Body>


we will have to change the taxonomy file for preservation of order and naming

src/components/SearchResults/Facets/FacetOption.js

src/components/SearchResults/Facets/FacetValue.js

src/components/SearchResults/SearchContext.js

…-3690

rayangler · 2023-10-05T18:18:03Z

@mmeigs

Should there be downward arrows to the left of these option groupings that have hidden expansions? This could be a later ticket, just wondering. I'll attach an example from the figma.
Note: it's a bit confusing because in the figma all the examples are for versions (which are ignored here, I know), but I'm wondering if options such as "Atlas" which have so many other options nested should have the arrows and option to open it up without selecting it.

Updated the implementation to have Atlas's subfacets always shown as requested by design. Iz mentioned that the arrow to the left should only be shown to display versions for a facet (like Atlas CLI, but this will be addressed separately). We want Atlas and its subfacets to always be shown, so we don't want that caret/arrow present. Other facets that have subfacets will only show their subfacets when the parent is clicked on (like Realm).

TOTAL nit. But I had a slow brain moment for a while looking back and forth between FacetOption and FacetValue. Thinking there might be a better naming convention? I do not have perfect "options" here but something like FacetOption -> FacetGroup(ing) and FacetValue -> FacetOption? Not sure. Also, disregard if you like!!

Yup, I tried to name the components based on the data types received in the response. That being said, I do like using FacetGroup instead of FacetOption. I think it might make sense to keep FacetValue named as is for now to minimize overlapping terminology between component and data (i.e. FacetOption doesn't represent facet-option but facet-value)

seungpark

this largely LGTM.

on a minor note, should we be resetting facet selections on a new search query? or persist user selected facets

seungpark · 2023-10-05T19:48:20Z

src/components/SearchResults/Facets/FacetValue.js

  }
 `;

-const FacetValue = ({ facetValue: { name, facets, key, id } }) => {
-  const { handleFacetChange, selectedFacets } = useContext(SearchContext);
+const initChecked = (searchParams, key, id) => searchParams.getAll(`facets.${key}`).includes(id);


minor. we can export FACETS_KEY_PREFIX from SearchContext

Good idea. Done

src/components/SearchResults/Facets/FacetGroup.js

rayangler · 2023-10-05T21:11:57Z

on a minor note, should we be resetting facet selections on a new search query? or persist user selected facets

Current search implementation in prod shows that filters are kept between searches. Let's check back in with design to confirm expected behavior and handle separately if needed

seungpark · 2023-10-06T17:20:02Z

src/components/SearchResults/SearchResults.js

the useEffect actually has to account for facets now 🤦 we totally overlooked this. was wondering why the results was not refreshing.

this should also be distinguished between the showFacets or not property

Spoke briefly about this offline. This PR intentionally omitted this as the initial UI implementation's goal was to focus on displaying facets. We can have a separate ticket or PR as needed to handle successful queries

mmeigs

LGTM!! Great work. So nice.
For posterity, putting here that (although very rare) I suppose I worry a bit about max url size in how we're using these params. If a user were to click idk like 70% of the facets so far (and in the future with versions, it would be less) the max char limit of 2,083 for a url will be hit.
Once again, don't think this will be an issue, but want to voice it.

rayangler · 2023-10-06T21:28:23Z

For posterity, putting here that (although very rare) I suppose I worry a bit about max url size in how we're using these params. If a user were to click idk like 70% of the facets so far (and in the future with versions, it would be less) the max char limit of 2,083 for a url will be hit.

This is a good callout. Seems like this limit is browser-dependent and it seems like we're fine for now. I agree we should definitely be mindful of how long our urls get. Let's revisit as we finalize the UI and data

rayangler added 9 commits September 27, 2023 16:48

DOP-3690: Initial implementation of Search Facets UI

629595f

Fix spacing

4b42673

Show mobile filters

6e60829

Add tests

306238f

Lint

01f7fd0

Remove unused style

442407e

Merge branch 'master' of https://github.com/mongodb/snooty into DOP-3690

f83066a

Consolidate test data for initial implementation

9adfd9f

Add comments

f3ca309

rayangler marked this pull request as ready for review September 29, 2023 15:08

rayangler requested review from seungpark and mmeigs September 29, 2023 15:08

mmeigs reviewed Sep 29, 2023

View reviewed changes

seungpark reviewed Oct 2, 2023

View reviewed changes

src/components/SearchResults/Facets/FacetValue.js Outdated Show resolved Hide resolved

Indeterminate state for Atlas product

f59b8d4

seungpark reviewed Oct 4, 2023

View reviewed changes

src/components/SearchResults/SearchContext.js Outdated Show resolved Hide resolved

rayangler and others added 9 commits October 4, 2023 10:29

More or less

a273444

DOP-3690: remove selectedFacets as a state. read from url params (#931)

0e3b144

Fix duplicate keys

cd9d1d4

Merge branch 'DOP-3690' of https://github.com/mongodb/snooty into DOP…

b654400

…-3690

Fix up validation for nested subfacets

7641536

Address feedback

25682c0

Merge branch 'master' of https://github.com/mongodb/snooty into DOP-3690

f5f1951

Fix parent query param

1e5e1a2

Fix tests and broken effect

5349cf9

Merge branch 'master' of https://github.com/mongodb/snooty into DOP-3690

6d54bab

rayangler requested a review from seungpark October 5, 2023 18:21

rayangler requested a review from mmeigs October 5, 2023 18:21

seungpark approved these changes Oct 5, 2023

View reviewed changes

Address feedback

36cc7c2

seungpark reviewed Oct 6, 2023

View reviewed changes

mmeigs approved these changes Oct 6, 2023

View reviewed changes

Merge branch 'master' into DOP-3690

0969be0

rayangler merged commit 1289447 into master Oct 6, 2023

rayangler deleted the DOP-3690 branch October 6, 2023 21:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DOP-3690: Initial implementation of Search Facets UI #925

DOP-3690: Initial implementation of Search Facets UI #925

rayangler commented Sep 29, 2023 •

edited

Loading

mmeigs left a comment

mmeigs Sep 29, 2023

rayangler Oct 5, 2023

mmeigs Sep 29, 2023

rayangler Oct 5, 2023

mmeigs Sep 29, 2023

rayangler Oct 5, 2023 •

edited

Loading

mmeigs Sep 29, 2023

seungpark Oct 2, 2023

seungpark left a comment

seungpark Oct 2, 2023

rayangler Oct 5, 2023

seungpark Oct 2, 2023

rayangler commented Oct 5, 2023 •

edited

Loading

seungpark left a comment

seungpark Oct 5, 2023

rayangler Oct 5, 2023

rayangler commented Oct 5, 2023

seungpark Oct 6, 2023

rayangler Oct 6, 2023

mmeigs left a comment

rayangler commented Oct 6, 2023

DOP-3690: Initial implementation of Search Facets UI #925

DOP-3690: Initial implementation of Search Facets UI #925

Conversation

rayangler commented Sep 29, 2023 • edited Loading

Stories/Links:

Current Behavior:

Staging Links:

Notes:

mmeigs 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

rayangler Oct 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seungpark 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

rayangler commented Oct 5, 2023 • edited Loading

seungpark left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rayangler commented Oct 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmeigs left a comment

Choose a reason for hiding this comment

rayangler commented Oct 6, 2023

rayangler commented Sep 29, 2023 •

edited

Loading

rayangler Oct 5, 2023 •

edited

Loading

rayangler commented Oct 5, 2023 •

edited

Loading