Conversation
✅ Deploy Preview for regen-website ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for terrasos ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
@erikalogie @S4mmyb see testing instructions |
|
LGTM |
r41ph
left a comment
r41ph
left a comment
There was a problem hiding this comment.
The implementation looks good, I'm approving these changes with just a couple of minor comments in the code. Also, as I've mentioned, I wonder if we really need all this additional logic for applying the filters after clicking 'Apply filters' on mobile.
The initial issue was about having an awkward UX because the filters were applied behind the modal and the users had to manually close the modal to be able to see the results. The current solution introduces more code to maintain, and users still need to click a button and wait for filters to apply, which they didn't have to before.
Would it make sense to reconsider if this extra complexity is necessary, or if we could achieve a similar UX with a simpler approach? For example, instead of the X (close button) we could have a single button bellow that changes its state from 'close' (if no filters have changed) to 'see projects' when filters are modified?
|
|
||
| export const getCreditClassSelectedFilters = ( | ||
| creditClassSelectedFilters: Record<string, boolean>, | ||
| export const getCreditClassFilters = ( |
There was a problem hiding this comment.
A more descriptive name might be setAllCreditClassFilters because the function is setting every filter to a given value
There was a problem hiding this comment.
I've just dropped the "Selected" in this function name
With the way we use it (in a setter function), changing it might look a bit repetitive, no?
setCreditClassFilters(setAllCreditClassFilters(creditClassSelectedFilters, false));
but I agree we could find a better name
| 'filterPermissionlessCredits', | ||
| { | ||
| selected: getFilterSelected(event.target.checked), | ||
| selected: getFilterSelected(checked), |
There was a problem hiding this comment.
Since it returns a filter status, I'd consider renaming this function to something like getFilterStatus
| getCreditClassSelectedFilters(creditClassSelectedFilters, true), | ||
| ); | ||
| if (selectAllEnabled) { | ||
| if (mobile) { |
There was a problem hiding this comment.
We could avoid repetition by creating a helper function:
...
const updateFilters = (newState: boolean) => {
const updatedFilters = getCreditClassFilters(creditClassSelectedFilters, newState);
if (mobile) {
setTempCreditClassFilters(updatedFilters);
} else {
setCreditClassFilters(updatedFilters);
}
};
...
and then in both Subtitle set the right state
...
onClick={() => {
if (selectAllEnabled) {
updateFilters(true);
}
}}
...
From what we had discussed with @erikalogie, she wanted the filters to be applied only after clicking the "apply filters" button, hence the need for storing an additional temporary state for the filters but open to update this if you think this adds too many clicks @erikalogie |
This implementation looks good to me, though I understand what Ralph is saying. |
|
Both UX/UI and technical implementations look good. What I wonder is whether we need all the logic behind applying the filters at the end of the selection process (which is code that we'll need to maintain and that adds complexity) or if we could achieve the same UX improvements with minor UI changes, as I mentioned in my previous comment. Arguably, applying filters immediately behind the modal, as we already do, is a better UX experience because users don’t have to wait when they click 'Apply Filters', as the results are already updated. |
|
@erikalogie any thoughts? |
|
Hey @erikalogie in a chat with @blushi she pointed out that one of the UI/UX requirements for this feature is that if a user selects a filter(s) and then clicks close, those filters should not be applied. I wasn't aware of that when I shared my thoughts about this feature. With this requirement in mind, my previous point may no longer be as relevant. |
|
@r41ph ah ok, makes sense. Thanks for always thinking through this UX stuff and bringing up good ideas. Let's leave this as-is in this case. |
blushi
force-pushed
the
feat-APP-545-mobile-projects-filters
branch
from
47dcc58 to
c86bfcf
Compare
3 participants
Description
Closes: https://regennetwork.atlassian.net/browse/APP-545
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
How to test
From https://deploy-preview-2595--regen-marketplace.netlify.app/projects/1
Reviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...