feat: 512/unit type filtering by cade-exygy · Pull Request #571 · metrotranscom/doorway

cade-exygy · 2024-04-05T18:51:04Z

feat: 512/unit type filtering

Issue Overview

https://app.zenhub.com/workspaces/bloom-5dc32d7144bd400001315dac/issues/gh/metrotranscom/doorway/512

This PR addresses #512

This change addresses the issue in full
This change addresses only certain aspects of the issue
This change is a dependency for another issue
This change has a dependency from another issue

Description

Allow searching for multiple bedroom and bathroom values at once
Pull in new CheckboxGroup component from Seeds: feat: 512 CheckboxGroup bloom-housing/ui-seeds#87

How Can This Be Tested/Reviewed?

Go to listings -> filter, and try out bedroom/bathroom filters
**- Currently the seed data on localhost has incorrect database values for bedrooms, which are different from the unit type. This causes the filtering to appear not to align properly. **

Describe the tests that you ran to verify your changes. Please also list any relevant details for your test configuration.

Checklist:

Reviewer Notes:

Steps to review a PR:

Read and understand the issue, and ensure the author has added QA notes
Review the code itself from a style point of view
Pull the changes down locally and test that the acceptance criteria is met
Also review the acceptance criteria on the Netlify deploy preview (noting that these do not yet include any backend changes made in the PR)
Either explicitly ask a clarifying question, request changes, or approve the PR if there are small remaining changes but the PR is otherwise good to go

On Merge:

If you have one commit and message, squash. If you need each message to be applied, rebase and merge.

…inclusive or filter

emilyjablonski · 2024-05-07T22:12:59Z

@cade-exygy Heads up about the failing tests

emilyjablonski · 2024-05-08T01:43:21Z

+}
+
+.checkbox-group > div > label {
+  color: var(--seeds-color-primary-dark) !important;


We try to do everything we can to not use important, it can make things brittle / the order of CSS application becomes confusing. If this is the root class of the new component in seeds, if we're needing to override a style over here, we should make it a CSS variable over there :)

emilyjablonski · 2024-05-08T01:47:36Z

  })
-  @IsNumberString({}, { groups: [ValidationsGroupsEnum.default] })
-  [ListingFilterKeys.bedrooms]?: number;
+  @IsArray({ groups: [ValidationsGroupsEnum.default] })


@YazeedLoonat Do you have preferences on stronger typing here?

emilyjablonski · 2024-05-08T02:11:11Z

I see the label so I did take a look, but not sure it's ready for review? It doesn't look like filtering is working for me locally, and if I try and open the filter modal on the listings page I get a react error. Pls let me know if I am missing something!

cade-exygy · 2024-05-08T16:04:38Z

I see the label so I did take a look, but not sure it's ready for review? It doesn't look like filtering is working for me locally, and if I try and open the filter modal on the listings page I get a react error. Pls let me know if I am missing something!

Hmm, could you share the react error you are getting? I just merged latest main and ran locally and am not seeing an error

emilyjablonski · 2024-05-08T19:52:52Z

Ty for the merge! Post merge I no longer get it opening the filter modal, but I get it trying to visit the partners site. The failing partners Cypress tests are caused by the same issue.

emilyjablonski · 2024-05-08T19:58:29Z

A few more notes for ya, I am still curious if filtering is working for you locally though?

I'm not seeing the new multiselect on the homepage
The Figma indicates we need to remove the Any option on the homepage and does still have all caps on the filter modal
If I select Studio and hit View listings the listings are not filtered
It appears like one filter might be set from the filter button but when I open it, nothing is set
If I select a unit filter, not all of the listings that match that filter seem to be coming back (video below)

filtering.mov

cade-exygy · 2024-06-12T23:26:52Z

@sarahlazarich @emilyjablonski I addressed what I could on this PR with my limited capacity.

Filters should work on Landing Search and Listings view
Refactored to reduce repetitive code

Remaining:

I believe the seed data has mismatches between the # of bedrooms and the associated unit type. Meaning that the filters come back with odd looking results.
Style changes: I changed the checkbox inputs to match more closely to the other form inputs.
Having some issues with tests. May be something to do with the ui-seeds version I am specifying?

If possible, I think pairing to knock out these last few remaining issues would be best path forward due to my limited time.

Thank you for your patience!

emilyjablonski · 2024-06-13T17:10:07Z

Ty for the update! Would be interested in green tests before the next review round, happy to pair

cade-exygy · 2024-06-13T17:17:26Z

Ty for the update! Would be interested in green tests before the next review round, happy to pair

I am uncertain why Partners tests are failing, at first glance it doesn't seem related to my changes, but curious if it has to do with the specified version of ui-seeds?

emilyjablonski · 2024-06-13T22:08:55Z

@cade-exygy Not sure w/out digging into it, but v happy to pair if it is sticky!

cade-exygy · 2024-06-17T19:06:52Z

Tests passing. Needs 2 reviews. If there any follow-ups (that are not small tweaks), I will need to hand off to another engineer, as I have no capacity for Doorway work currently. Let me know if there any questions. Thank you! cc. @sarahlazarich

mcgarrye

Testing locally, finding that the filtering results are not always accurate

mcgarrye · 2024-06-27T16:07:07Z

+  --inner-button-gap: var(--seeds-s3);
+  padding-top: var(--seeds-s4);
+  padding-bottom: var(--seeds-s4);
+}


nit: missing EOF newline

cade-exygy · 2024-06-27T17:14:02Z

Testing locally, finding that the filtering results are not always accurate

Curious what your local database data looks like? On my end I saw that the seed data has lots of misalignments between # of bedrooms and Unit type label. For example a listing with 0 bedrooms is actually labeled as "1 bedroom" etc.

mcgarrye · 2024-06-27T17:35:36Z

Testing locally, finding that the filtering results are not always accurate

Curious what your local database data looks like? On my end I saw that the seed data has lots of misalignments between # of bedrooms and Unit type label. For example a listing with 0 bedrooms is actually labeled as "1 bedroom" etc.

Great call out cause that's exactly what it was!

mcgarrye

We should definitely have a follow-up ticket to fix the seed data

jaredcwhite

The new functionality seems to be working well for me, but I'm noticing a visual regression where it looks like a stock Seeds component's been dropped into the Doorway design system.

I suspect some overridden tokens are needed?

cade-exygy added 3 commits March 19, 2024 11:03

feat(listing service search): change query construction to allow for …

532446d

…inclusive or filter

Merge branch 'main' into 512/unit-type-filtering

c057b45

feat: create ButtonCheckboxGroup component

6293766

cade-exygy added the on hold label Apr 5, 2024

cade-exygy requested review from ColinBuyck, cliu02 and emilyjablonski April 5, 2024 18:51

cade-exygy commented Apr 5, 2024

View reviewed changes

Comment thread api/src/services/listing.service.ts Outdated

cade-exygy commented Apr 5, 2024

View reviewed changes

Comment thread api/src/services/listing.service.ts Outdated

cade-exygy commented Apr 5, 2024

View reviewed changes

Comment thread sites/public/src/components/listings/search/ButtonSelect.tsx Outdated

cade-exygy added 3 commits May 7, 2024 16:08

feat: uptake seeds and use CheckboxGroup

c71f19d

chore: merge main

2d51b75

refactor(remove buttonselect): remove ButtonSelect

ac4ec19

cade-exygy requested review from YazeedLoonat, jaredcwhite and mcgarrye May 7, 2024 21:42

cade-exygy added 2 reviews needed and removed on hold labels May 7, 2024

Merge branch 'main' into 512/unit-type-filtering

2c2728e

emilyjablonski reviewed May 8, 2024

View reviewed changes

Comment thread api/src/services/listing.service.ts

emilyjablonski reviewed May 8, 2024

View reviewed changes

emilyjablonski added needs changes and removed 2 reviews needed labels May 8, 2024

Merge branch 'main' into 512/unit-type-filtering

2eabdda

cade-exygy and others added 10 commits June 5, 2024 17:39

Merge branch 'main' into 512/unit-type-filtering

a3039a5

refactor: create helper for unit filters

af3904c

feat: checkboxGroup on LandingSearch

1f4c683

Merge branch 'main' into 512/unit-type-filtering

6622394

fix: fix import

03212ce

fix: test fix seeds intake

800a997

Merge branch 'main' into 512/unit-type-filtering

91afb0f

fix: seeds version and add padding

4b8c730

fix: latest seeds

4d27c15

fix: fix bedroom type

cd025a9

cade-exygy requested a review from emilyjablonski June 12, 2024 23:27

cade-exygy added 2 reviews needed and removed needs changes labels Jun 12, 2024

cade-exygy and others added 3 commits June 17, 2024 13:42

Merge branch 'main' into 512/unit-type-filtering

9141179

fix: latest seeds after merge

9bb15b0

Merge branch 'main' into 512/unit-type-filtering

9502659

mcgarrye reviewed Jun 27, 2024

View reviewed changes

mcgarrye approved these changes Jun 27, 2024

View reviewed changes

mcgarrye added 1 review needed and removed 2 reviews needed labels Jun 27, 2024

jaredcwhite suggested changes Jun 28, 2024

View reviewed changes

jaredcwhite added needs changes and removed 1 review needed labels Jun 28, 2024

Uh oh!

Conversation

cade-exygy commented Apr 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

feat: 512/unit type filtering

Issue Overview

Description

How Can This Be Tested/Reviewed?

Checklist:

Reviewer Notes:

On Merge:

Uh oh!

Uh oh!

Uh oh!

Uh oh!

emilyjablonski commented May 7, 2024

Uh oh!

emilyjablonski May 8, 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!

Uh oh!

emilyjablonski May 8, 2024

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

cade-exygy commented May 8, 2024

Uh oh!

emilyjablonski commented May 8, 2024

Uh oh!

emilyjablonski commented May 8, 2024

Uh oh!

cade-exygy commented Jun 12, 2024

Uh oh!

emilyjablonski commented Jun 13, 2024

Uh oh!

cade-exygy commented Jun 13, 2024

Uh oh!

emilyjablonski commented Jun 13, 2024

Uh oh!

cade-exygy commented Jun 17, 2024

Uh oh!

mcgarrye left a comment

Choose a reason for hiding this comment

Uh oh!

mcgarrye Jun 27, 2024

Choose a reason for hiding this comment

Uh oh!

cade-exygy commented Jun 27, 2024

Uh oh!

mcgarrye commented Jun 27, 2024

Uh oh!

mcgarrye left a comment

Choose a reason for hiding this comment

Uh oh!

jaredcwhite left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

cade-exygy commented Apr 5, 2024 •

edited

Loading

emilyjablonski May 8, 2024 •

edited

Loading

emilyjablonski commented May 8, 2024 •

edited

Loading