Add showMoreLimit prop #245

ElemelonWind · 2022-07-19T15:47:20Z

Add a showMoreLimit prop to StandardFacet. When the number of options exceeds this limit, display only that many options, a searchable facets input, and a show more button. The show more button, when clicked displays all the options. If the number of options is less than the limit, show all the options and no search bar

J=SLAP-2252
TEST=auto

Added two new stories: one to show the prop at work, and the second to show what happens when the show more button is pressed

Note: Coveralls decreased by 0.3% because it doesn't count stories as test coverage

coveralls · 2022-07-19T15:51:56Z

Coverage decreased (-0.1%) to 84.291% when pulling 34a9b68 on dev/standardfacets-showmorelimit into 21087c8 on main.

…y notices check

tests/components/StandardFacets.stories.tsx

src/components/FilterGroup.tsx

etc/search-ui-react.api.md

oshi97 · 2022-07-21T15:15:55Z

src/components/FilterGroup.tsx

+  const limited = filterOptions.length > showMoreLimit;
+  const [showAll, setShowAll] = useState<boolean>(!limited);
+
+  function toggleShowAll() {


At first I thought we might want to wrap this in a useCallback, if only for consistency, but after thinking about it more we can probably omit it.

Out of curiosity, if you use an arrow function instead like onClick={e => setShowAll(!showAll)} does a react-perf eslint rule complain? I was thinking that it might be better to use an arrow function and an eslint-ignore comment, just to be explicit here.

Since toggleShowAll is just passed into a regular button and not another component I don't think there's benefits of wrapping this in useCallback

oshi97 · 2022-07-21T16:31:34Z

src/components/FilterGroup.tsx

@@ -104,7 +100,8 @@ export function FilterGroup({
            />
          );
        })}
-        {limited && <button className='text-blue-500 py-1 text-sm' onClick={toggleShowAll}>{showAll ? 'Show Less' : 'Show More'}</button>}
+        {/* eslint-disable-next-line react-perf/jsx-no-new-function-as-prop */}
+        {limited && <button className='text-primary py-1 text-sm' onClick={_e => setShowAll(!showAll)}>{showAll ? 'Show Less' : 'Show More'}</button>}


Can this be multiple lines? You can also replace the _e with ()

not sure if I formatted it right lol, could you take a look?

…/yext/search-ui-react into dev/standardfacets-showmorelimit merge.

nmanu1 · 2022-07-21T18:18:37Z

src/components/FilterGroup.tsx

@@ -85,7 +91,7 @@ export function FilterGroup({
      {renderTitle()}
      <CollapsibleSection className={cssClasses.optionsContainer}>
        {searchable && <SearchInput className={cssClasses.searchInput} />}
-        {filterOptions.map(o => {
+        {filterOptions.slice(0, showAll ? filterOptions.length : showMoreLimit).map(o => {


did you test if this works properly when typing in the search input? it seems like the search would only be performed on this slice of filter options instead of searching all of the options and then displaying the first few

typing in the search input should also change whether show more/less is displayed depending on how many options match the input text

alextaing · 2022-07-22T18:04:07Z

src/components/FilterGroup.tsx

@@ -94,6 +100,11 @@ export function FilterGroup({
            />
          );
        })}
+        {limited &&
+        /* eslint-disable-next-line react-perf/jsx-no-new-function-as-prop */
+        <button className='text-primary py-1 text-sm' onClick={() => setShowAll(!showAll)}>


Instead of using the eslint disable, can we extract the function and call it in onClick?

I wanted to specifically use the eslint disable instead of using useCallback.

Extracting it to a function doesn't actually give it a stable reference, you have to use useCallback or pull the function out to the top level.
I'm fine with useCallback as well I just think it hurts performance a little because we don't actually need the memoization since this is going into a default button element and not another component (even then it's very arguable but we've been doing it everywhere so should be consistent for now and refactor everything later if we decide otherwise). But performance is not really a concern here! useCallback also sgtm

ohhh I see-- I totally missed that comment earlier, that's fine we can totally leave it haha

@yen-tt @nmanu1 do you guys prefer useCallback here? I'm really 50/50 and it's probably not a decision worth stressing over but just wondering

I think I have a slight preference for not using useCallback here. just because it's not really needed and I'm in favor of not wrapping in useMemo/useCallback unless it adds some value

alextaing · 2022-07-22T18:08:12Z

src/components/FilterGroup.tsx

@@ -69,6 +72,9 @@ export function FilterGroup({
    };
  }, [customCssClasses]);

+  const limited = filterOptions.length > showMoreLimit;


Super subjective, but since this is a boolean it might be a tad more legible for the future if it were called isLimited-- but it's tiny haha

oshi97 · 2022-07-22T18:43:41Z

src/components/FilterGroup.tsx

@@ -94,6 +100,11 @@ export function FilterGroup({
            />
          );
        })}
+        {limited &&
+        /* eslint-disable-next-line react-perf/jsx-no-new-function-as-prop */


I think it's a little nicer if we indent everything after the && in this conditional (lines 104-107) and move the last brace to the next line

ElemelonWind · 2022-07-22T18:57:43Z

I'll be removing the shouldRenderOption logic from CheckboxOption and putting it into CheckboxOptions after demos but wanted to push in case people had any feedback for the new internal component

oshi97 · 2022-07-22T20:36:14Z

src/components/FilterGroup.tsx

        {children}
      </CollapsibleSection>
    </FilterGroupProvider>
  );
 }
+
+function CheckboxOptions({
+  filterOptions,


Do these parameters have type any if you don't specify a type?

Should I make an interface for it?

You could also specify it inline

like { myParam }: { myParam: string }

oshi97 · 2022-07-22T20:37:53Z

src/components/FilterGroup.tsx

+
+  return (
+    <>
+      {filterOptions.filter(o => {


I think this can just be filterOptions.filter(shouldRenderOption)

oshi97 · 2022-07-22T20:38:21Z

src/components/FilterGroup.tsx

+}) {
+  const searchUtilities = useSearchUtilities();
+  const { searchValue } = useFilterGroupContext();
+  const shouldRenderOption = useCallback((option: { displayName: string, value: unknown }) => {


does this need a useCallback? We're not passing this function anywhere

oshi97 · 2022-07-22T20:39:55Z

src/components/FilterGroup.tsx

        {children}
      </CollapsibleSection>
    </FilterGroupProvider>
  );
 }
+
+function CheckboxOptions({


Would it make sense to put this component in a separate file?

Tom said since it's an internal component we can just keep it in the same file

tmeyer2115 · 2022-07-25T14:53:27Z

src/components/FilterGroup.tsx

@@ -119,7 +120,12 @@ function CheckboxOptions({
 }) {
  const searchUtilities = useSearchUtilities();
  const { searchValue } = useFilterGroupContext();
-  const shouldRenderOption = useCallback((option: { displayName: string, value: unknown }) => {
+  const shouldRenderOption = (option:
+  { displayName?: unknown, value: string | number | boolean | NumberRangeValue }) => {


Instead of unknown, could we do string | undefined here? I can't remember off the top of my head, but we may also be able to do displayName?: string. The string | undefined may be redundant because of the optional nature of `displayName.

tmeyer2115 · 2022-07-25T14:58:57Z

src/components/FilterGroup.tsx

@@ -119,7 +120,12 @@ function CheckboxOptions({
 }) {
  const searchUtilities = useSearchUtilities();
  const { searchValue } = useFilterGroupContext();
-  const shouldRenderOption = useCallback((option: { displayName: string, value: unknown }) => {
+  const shouldRenderOption = (option:


@oshi97 do we not want to wrap this in a useMemo or a useCallback? I'm wondering if, as is, we could erroneously re-render if searchValue and searchUtilities remained the same.

src/components/FilterGroup.tsx

nmanu1 · 2022-07-25T16:47:33Z

src/components/FilterGroup.tsx

+        {isLimited &&
+          /* eslint-disable-next-line react-perf/jsx-no-new-function-as-prop */
+          <button className='text-primary py-1 text-sm' onClick={() => setShowAll(!showAll)}>
+            {showAll ? 'Show Less' : 'Show More'}


would this scenario be handled correctly?

filterOptions > showMoreLimit, so the options are searchable and have a Show More button displayed

user types in the input which narrows down the matching options to fewer than showMoreLimit

there probably shouldn't be a Show More/Less button displayed anymore

src/components/FilterGroup.tsx

Follow-up to (#245) - jest tests didn't catch the bug :(

Add a `showMoreLimit` prop to `StandardFacet`. When the number of options exceeds this limit, display only that many options, a searchable facets input, and a show more button. The show more button, when clicked displays all the options. If the number of options is less than the limit, show all the options and no search bar J=SLAP-2252 TEST=auto Added two new stories: one to show the prop at work, and the second to show what happens when the show more button is pressed

Follow-up to (#245) - jest tests didn't catch the bug :(

ElemelonWind and others added 2 commits July 19, 2022 11:44

added prop

1a05170

Automated update to repo's documentation from github action

013e993

ElemelonWind and others added 4 commits July 19, 2022 14:39

pass prop to filtergroup

3312b52

Automated update to THIRD-PARTY-NOTICES from github action's 3rd part…

8671571

…y notices check

added show more button

48ddb47

storybook tests

af100b3

ElemelonWind marked this pull request as ready for review July 21, 2022 14:15

ElemelonWind requested a review from a team as a code owner July 21, 2022 14:15

nmanu1 reviewed Jul 21, 2022

View reviewed changes

tests/components/StandardFacets.stories.tsx Outdated Show resolved Hide resolved

src/components/FilterGroup.tsx Outdated Show resolved Hide resolved

resolve redundancies

6a9ce8a

yen-tt reviewed Jul 21, 2022

View reviewed changes

src/components/FilterGroup.tsx Outdated Show resolved Hide resolved

yen-tt reviewed Jul 21, 2022

View reviewed changes

etc/search-ui-react.api.md Show resolved Hide resolved

oshi97 reviewed Jul 21, 2022

View reviewed changes

ElemelonWind and others added 2 commits July 21, 2022 12:18

change button class, onclick, omit field

8646f22

Automated update to repo's documentation from github action

8b9d5d3

oshi97 reviewed Jul 21, 2022

View reviewed changes

ElemelonWind added 2 commits July 21, 2022 12:57

reformatting

81b461f

Merge branch 'dev/standardfacets-showmorelimit' of https://github.com…

915228a

…/yext/search-ui-react into dev/standardfacets-showmorelimit merge.

nmanu1 reviewed Jul 21, 2022

View reviewed changes

alextaing reviewed Jul 22, 2022

View reviewed changes

oshi97 reviewed Jul 22, 2022

View reviewed changes

checkboxoptions to access context

08a58cd

cleanup of shouldRenderOption

9589ed8

ElemelonWind added the wip label Jul 22, 2022

oshi97 reviewed Jul 22, 2022

View reviewed changes

ElemelonWind added 2 commits July 25, 2022 10:01

fix type checking

7966e0f

fix lint issue

65d5e44

ElemelonWind removed the wip label Jul 25, 2022

add types interface to checkboxoptions

b3f2673

tmeyer2115 reviewed Jul 25, 2022

View reviewed changes

displayName as string

bb1eb60

yen-tt reviewed Jul 25, 2022

View reviewed changes

src/components/FilterGroup.tsx Outdated Show resolved Hide resolved

replace unions

b4fa488

yen-tt reviewed Jul 25, 2022

View reviewed changes

src/components/FilterGroup.tsx Outdated Show resolved Hide resolved

code cleanup

c3aa08d

yen-tt reviewed Jul 25, 2022

View reviewed changes

src/components/FilterGroup.tsx Outdated Show resolved Hide resolved

nmanu1 reviewed Jul 25, 2022

View reviewed changes

src/components/FilterGroup.tsx Outdated Show resolved Hide resolved

code cleanup

9b317cd

nmanu1 reviewed Jul 25, 2022

View reviewed changes

remove showmore button when search returns less than limit

1c038b0

nmanu1 reviewed Jul 25, 2022

View reviewed changes

src/components/FilterGroup.tsx Outdated Show resolved Hide resolved

src/components/FilterGroup.tsx Outdated Show resolved Hide resolved

moved some variables to checkboxoptions component

822b64f

nmanu1 reviewed Jul 25, 2022

View reviewed changes

src/components/FilterGroup.tsx Outdated Show resolved Hide resolved

ElemelonWind added 2 commits July 25, 2022 14:31

address nit

dc34ce6

fix lint issue

34a9b68

tmeyer2115 self-requested a review July 25, 2022 18:44

tmeyer2115 approved these changes Jul 25, 2022

View reviewed changes

ElemelonWind merged commit d4c0f98 into main Jul 25, 2022

ElemelonWind deleted the dev/standardfacets-showmorelimit branch July 25, 2022 18:45

ElemelonWind mentioned this pull request Jul 25, 2022

Fixed bug that tried to set a read-only dict #254

Merged

ElemelonWind added a commit that referenced this pull request Jul 25, 2022

Fixed bug that tried to set a read-only dict (#254)

d081a46

Follow-up to (#245) - jest tests didn't catch the bug :(

yen-tt pushed a commit that referenced this pull request Sep 28, 2022

Fixed bug that tried to set a read-only dict (#254)

506cd5c

Follow-up to (#245) - jest tests didn't catch the bug :(

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add showMoreLimit prop #245

Add showMoreLimit prop #245

ElemelonWind commented Jul 19, 2022 •

edited

coveralls commented Jul 19, 2022 •

edited

oshi97 Jul 21, 2022

oshi97 Jul 21, 2022

ElemelonWind Jul 21, 2022

nmanu1 Jul 21, 2022 •

edited

alextaing Jul 22, 2022

oshi97 Jul 22, 2022 •

edited

alextaing Jul 22, 2022 •

edited

oshi97 Jul 22, 2022

nmanu1 Jul 22, 2022

alextaing Jul 22, 2022 •

edited

oshi97 Jul 22, 2022

ElemelonWind commented Jul 22, 2022

oshi97 Jul 22, 2022

ElemelonWind Jul 25, 2022

oshi97 Jul 25, 2022

oshi97 Jul 25, 2022

oshi97 Jul 25, 2022

oshi97 Jul 22, 2022

oshi97 Jul 22, 2022

oshi97 Jul 22, 2022

ElemelonWind Jul 25, 2022

tmeyer2115 Jul 25, 2022

tmeyer2115 Jul 25, 2022

nmanu1 Jul 25, 2022

Add showMoreLimit prop #245

Add showMoreLimit prop #245

Conversation

ElemelonWind commented Jul 19, 2022 • edited

coveralls commented Jul 19, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nmanu1 Jul 21, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oshi97 Jul 22, 2022 • edited

Choose a reason for hiding this comment

alextaing Jul 22, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alextaing Jul 22, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ElemelonWind commented Jul 22, 2022

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

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

ElemelonWind commented Jul 19, 2022 •

edited

coveralls commented Jul 19, 2022 •

edited

nmanu1 Jul 21, 2022 •

edited

oshi97 Jul 22, 2022 •

edited

alextaing Jul 22, 2022 •

edited

alextaing Jul 22, 2022 •

edited