[CI-3134] Filters Components #77

Mudaafi · 2024-05-09T03:05:17Z

Pull Request Checklist

Before you submit a pull request, please make sure you have to following:

I have added or updated TypeScript types for my changes, ensuring they are compatible with the existing codebase.
I have added JSDoc comments to my TypeScript definitions for improved documentation.
I have added tests that prove my fix is effective or that my feature works.
I have added any necessary documentation (if appropriate).
I have made sure my PR is up-to-date with the main branch.

PR Type

What kind of change does this PR introduce?

Bugfix
Feature
Code style update (formatting, local variables)
Refactoring (no functional changes, no API changes)
Documentation content changes
TypeScript type definitions update
Other... Please describe:

linear · 2024-05-09T03:05:19Z

CI-3134 [MVP] Create a Filters Component

Business Outcome:

As a Consumer I want Filters to be handled OOO so that I can fully port over my current implementation to use Constructor's OS UI Library

Definition of done:

Utilize the useFilters Hook
Takes in a Search Response object and renders the Filters
- Should be able to render: multiple, range and range buckets
Tests
Documentation

Additional Notes:

https://www.figma.com/file/bWsxaLJGgHTX9ystM0Ko5Z/Open-Source-Demo?type=design&node-id=1%3A1317&mode=design&t=kL2O8RQZtPrsaepq-1

Screenshot 2023-12-23 at 12.55.42 AM.png

HHHindawy

Looking good! Thanks for working on this!
Left you a couple of minor comments 🙏

HHHindawy · 2024-06-09T23:46:52Z

src/components/Filters/FilterOptionsList.tsx

+  if (optionsToRender.length === 0) return null;
+
+  return (
+    <div className={`cio-collapsible-wrapper${!isCollapsed ? ' cio-collapsible-is-open' : ''}`}>


A super minor thing to satisfy my OCD 😅
Can we add a space before ${!isCollapsed ? and remove the space here: ' cio-collapsible-is-open'

hmm, if we add the space outside the conditional, it'd be rendered even if isCollapsed == true tho. I'd rather avoid the extra space but I guess we can call a .trim() at the end.

Depending on how often these conditional classes come up, it might be worth bringing in a library like classnames if we want to keep things clean 😄

Otherwise, I don't have a strong opinion on either direction. If I had to choose, I'd go for keeping what we have already to keep the line of code shorter

+1 to adding classnames, it's really useful!

I'm down for adding classnames. It seems like we'll need to add it as an actual dependency but it should be fine since it's only 752 bytes when minified, 436 bytes minified + gzipped

src/components/Filters/FilterOptionsList.tsx

src/components/Filters/FilterOptionsList.css

src/components/Filters/FilterRangeSlider.css

HHHindawy · 2024-06-10T00:32:08Z

src/components/Filters/FilterRangeSlider.tsx

+  }, [visibleTrack]);
+
+  return (
+    <div className={`cio-collapsible-wrapper${!isCollapsed ? ' cio-collapsible-is-open' : ''}`}>


Same OCD comment 😅

spec/Filters.server.test.jsx

spec/local_examples/sampleFacets.json

src/components/Filters/FilterRangeSlider.css

src/stories/components/Filters/Documentation.mdx

src/components/Filters/FilterRangeSlider.tsx

lordvkrum · 2024-06-21T15:14:43Z

src/components/Filters/FilterRangeSlider.tsx

+            </span>
+            <div className='cio-slider-input'>
+              <span className='cio-slider-input-prefix'>to </span>
+              <input


small thing: should inputs have white-colot background, same as checkboxes do?

looking at the figma file, it may be the other way around... checkboxes shouldn't be white-color-bg?

hmm interesting. I didn't actually mean to specify the color of the inputs. I think it's fine to set it to white to keep it consistent. I'll make the changes

lordvkrum · 2024-06-21T15:26:18Z

src/components/Filters/FilterRangeSlider.tsx

+                value={inputMaxValue}
+                onChange={onMaxInputValueChange}


we can leave blank any of the inputs, but after doing that the slider is not updated... should we move the respective bar to the end of the range? (that's what empty means after all, right? no-min/max)

Not necessarily, it just means that the current value isn't set. The general experience is the filters re-apply after a change has been made, but in this state, we shouldn't reapply the filters, and the bar shouldn't move as well since it's an invalid state. I'd prefer either highlighting the border in red as we do for other invalid values, or making the corresponding sliding button slightly faded. wdyt?

is there no way to just set a min value and not max? (for example, search 30+)
if that's the expected behavior then yeah, I'd agree with adding the red-error state

Nope, we'll need to set both before applying the filter since it's a range. The min-max defaults to the actual min-max of the result set so 30+ would just mean modifying the minValue. Now that you mentioned it, if we update minInputValue, we don't actually check if the maxInputValue is valid before we run the callback 🤔 . I'll add a check for it.

src/components/Filters/FilterOptionsList.tsx

mocca102 · 2024-06-24T10:58:51Z

src/components/Filters/index.ts

+import Filters from './Filters';
+
+export * from './Filters';
+export default Filters;


Do we have a reason we're not exposing the other Filters components utilized by Filters? Sorry if this was already discussed

No particular reason, but I didn't see other components in the library needing to use them. Do you mean exposing it so that library users can take advantage of them?

mocca102

I see a lot of logic handled inside what should have been "Presentational Components" ie Components that should handle only UI.

This could be ok but let's assume I want to utilize the hooks and build the UI myself. All we're giving the users is useFilters(). We're not giving state nor click handler or anything else. So we're limiting the hook implementation by doing so.

I don't want us to handle this in this PR though if we're going to but lets have an internal sync on this. @Mudaafi

lordvkrum

LGTM

HHHindawy

LGTM!
Thanks for baring with us and making the changes!

[CI-3134] Filters Components

2e86553

Mudaafi added the wip Edits in Progress label May 9, 2024

Mudaafi added 9 commits May 18, 2024 05:09

Update mocks

ccdc528

Checkpoint 2 - Ranged Slider

4fdf1e8

Update to styles

eb0069a

CSS Updates - Options List

99e2537

Filter Group Header css

aa5f00a

Add tests

8a2c382

Merge branch 'main' into ci-3134-mvp-create-a-filters-component

fcba345

Fix Merge conflicts and update story

5973fd2

Add Documentation

2b26874

Mudaafi removed the wip Edits in Progress label Jun 3, 2024

Mudaafi added 2 commits June 3, 2024 23:57

lint

47a6de6

lint 2

c8997ae

Mudaafi requested review from a team and HHHindawy June 3, 2024 16:01

Mudaafi added 2 commits June 11, 2024 05:01

Only update filter-range after onMouseUp event

69e33e1

Increased debounced time, bugfix rerender calls

9aaf067

HHHindawy requested changes Jun 12, 2024

View reviewed changes

Mudaafi added 4 commits June 18, 2024 02:04

Fixes based on comments

61725d3

Add Classnames

6e6dc18

lint: remove unnecessary else statement

a2cb74d

bugfixes to remove console errors

f6a2fb5

Mudaafi requested review from HHHindawy, jjl014 and lordvkrum June 20, 2024 20:27

lordvkrum reviewed Jun 21, 2024

View reviewed changes

src/components/Filters/FilterOptionsList.tsx Show resolved Hide resolved

lordvkrum reviewed Jun 21, 2024

View reviewed changes

src/components/Filters/FilterOptionsList.tsx Show resolved Hide resolved

mocca102 reviewed Jun 24, 2024

View reviewed changes

Mudaafi added 7 commits June 25, 2024 02:44

Update styles

073ecb5

Update inputs to only update on blur

cd46155

Add more tests

31f4cea

Add new test

b40648b

Show invalid state if slider range inputs are empty

468a268

Fix input resizing on num digits increase

ee483b9

Change callback fn name, fix trackClick

b9fa20b

lordvkrum approved these changes Jun 25, 2024

View reviewed changes

CSS updates

54db633

HHHindawy approved these changes Jun 27, 2024

View reviewed changes

Updates placeholders and naming

2a90e89

Mudaafi merged commit 743645a into main Jun 27, 2024

Mudaafi deleted the ci-3134-mvp-create-a-filters-component branch June 27, 2024 18:12

[CI-3134] Filters Components #77

[CI-3134] Filters Components #77

Uh oh!

Conversation

Mudaafi commented May 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Checklist

PR Type

Uh oh!

linear bot commented May 9, 2024

Uh oh!

HHHindawy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Mudaafi Jun 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mocca102 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lordvkrum left a comment

Choose a reason for hiding this comment

Uh oh!

HHHindawy left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Mudaafi commented May 9, 2024 •

edited

Loading

Mudaafi Jun 24, 2024 •

edited

Loading

mocca102 left a comment •

edited

Loading