685: Bug Fix: Selected Panels 'Selected' State #688
Conversation
|
@jgrimes86 is attempting to deploy a commit to the Clean and Green Philly Team on Vercel. A member of the Team first needs to authorize it. |
|
@CodeWritingCow I think this PR is one for you to review |
|
Hi @jgrimes86 I tested your PR in my local env. When I toggle between the filter and property card views, the user's filter choices do persist. However, I encountered two unexpected behaviors: issue.685.01.mov
issue.685.02.mov
|
There was a problem hiding this comment.
@jgrimes86 can you take a look at the first behavior?
As for the second behavior, it might be caused by an issue unrelated to your current ticket. What do you think?
@CodeWritingCow I've pushed two commits to address both those issues. For the first behavior, I needed to update filterCount to reflect that three of the 'Get Access' buttons are different values of one 'access_process' filter. I think the second behaver is related to this ticket in that it is annoying that selected filters don't persist when you're using the website. But it might also be considered unrelated because it is caused by how filter context is saved, not by how the filter context is handled within the side panel components. Fixing that behavior seems to be a pretty straight-forward change, so I made a commit to address that issue also. The second behavior was happening because the FilterProvider, which provides FilterContext, was only providing context to the 'Find Properties' page components. So when you navigated to one of the other pages, anything stored in state in FilterContext was lost. I've moved FilterProvider into a new FilterProviderWrapper component, modelled after CookieProviderWrapper, and added that to RootLayout. Now you can navigate to another section of the website, and when you go back to 'Find Properties' the filters are still active. |
There was a problem hiding this comment.
Hi @jgrimes86 I ran your updated PR locally. You fixed both the first and second issues, and gave a detailed explanation of the second issue. Thanks for going above and beyond.
While the function filterCount fixes the filter count issue, it's getting called multiple times. (See screen recording; I added a console.log for each time that the UI calls filterCount). It'd be more performant if we call it only once or as needed. Otherwise, the rest of the PR looks good.
Can you update the PR so the UI calls filterCount only as needed?
issue.685.03.mov
|
@CodeWritingCow this most recent commit adds a useMemo to filterCount. I have the useMemo looking for changes to appFilter because filterCount needs to reflect changes to appFilter. Unfortunately, appFilter "changes" twice every time a filter is applied (although I can see no difference between those changes), which means that filterCount still gets called several times per filter toggle. But this revision means filterCount isn't called when you first go to the filter view, and I think it halves the amount of times filterCount is called when a filter is toggled. Video demo of console logs: |
|
@jgrimes86 I watched your screen recording and also went over the new commit with useMemo. The changes look good. I'll go ahead and approve this PR. 👍 |
This addresses issue #685
The selectedPanelKeys state, which controlled the appearance of selected panel filters is not saved when you toggle back and forth between the filter view and property cards view. To keep the panel filters correctly selected after toggling the view, the initial 'selectedPanelKeys' state is now generated from appFilter.
This is an example of the corrected panel filter appearance after toggling between the filter view and property cards view:
