Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[MPDX-8405] Fix filtering the map by selected contacts #1154

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

canac
Copy link
Contributor

@canac canac commented Oct 23, 2024

Description

  • Support filtering the contacts map by the selected contacts.
  • Refactor ContactsContext tests to use a reusable wrapper.

Testing

  • Case 1
    • Go to the contacts list and select a few contacts
    • Switch to the map view
    • Verify that only the selected contacts show up on the map
    • Switch back to the list view
    • Verify that all contacts show up in the list view (i.e. the list is not filtered by contact ids like the map was)
  • Case 2
    • Go to the contacts list (do not select any contacts)
    • Switch to the map view
    • Verify that all contacts show up on the map (after they finish loading)

MPDX-8405

Checklist:

  • I have given my PR a title with the format "MPDX-(JIRA#) (summary sentence max 80 chars)"
  • I have applied the appropriate labels. (Add the label "On Staging" to get the branch automatically merged into staging.)
  • I have requested a review from another person on the project

@canac canac added the Preview Environment Add this label to create an Amplify Preview label Oct 23, 2024
@canac canac requested a review from dr-bizz October 23, 2024 18:39
@canac canac self-assigned this Oct 23, 2024
Copy link

Preview branch generated at https://8405-filter-map.d3dytjb8adxkk5.amplifyapp.com

useUserPreference({
key: 'contacts_view',
defaultValue: TableViewModeEnum.List,
});
useEffect(() => {
if (contactsView && !userOptionsLoading) {
if (contactsView) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dr-bizz When I asked you to add this additional check, I was mistaken. I thought this effect saved the user preference, but it is just updating the view mode in the contacts wrapper. It was right how you had it first, so I'm switching it back.

Copy link

github-actions bot commented Oct 23, 2024

Bundle sizes [mpdx-react]

Compared against 9f1e8f2

Route Size (gzipped) Diff
/accountLists/[accountListId]/contacts/[[...contactId]] 109.73 KB +3.01 KB
/accountLists/[accountListId]/reports/donations/[[...contactId]] 348.14 KB +2.31 KB
/accountLists/[accountListId]/reports/partnerCurrency/[[...contactId]] 137.56 KB +3.14 KB
/accountLists/[accountListId]/reports/partnerGivingAnalysis/[[...contactId]] 135.87 KB +2.51 KB
/accountLists/[accountListId]/reports/salaryCurrency/[[...contactId]] 137.56 KB +3.14 KB
/accountLists/[accountListId]/tools 146.1 KB +3 KB
/accountLists/[accountListId]/tools/appeals 186.39 KB +2.81 KB
/accountLists/[accountListId]/tools/fix/emailAddresses/[[...contactId]] 166.23 KB +2.81 KB
/accountLists/[accountListId]/tools/fix/mailingAddresses/[[...contactId]] 124.35 KB +3 KB
/accountLists/[accountListId]/tools/fix/phoneNumbers/[[...contactId]] 147.48 KB +2.81 KB
/accountLists/[accountListId]/tools/fix/sendNewsletter/[[...contactId]] 122.25 KB +3 KB
/accountLists/[accountListId]/tools/import/csv 110.97 KB +3 KB
/accountLists/[accountListId]/tools/import/google 160.06 KB +2.81 KB
/accountLists/[accountListId]/tools/import/tnt 154.28 KB +2.8 KB
/accountLists/[accountListId]/tools/merge/contacts/[[...contactId]] 123.6 KB +3 KB
/accountLists/[accountListId]/tools/merge/people/[[...contactId]] 122.89 KB +3 KB
Dynamic import Size (gzipped) Diff
../src/components/Contacts/ContactFlow/DynamicContactFlow.tsx -> ./ContactFlow 64.46 KB -3.1 KB
../src/components/Contacts/ContactsList/DynamicContactsList.tsx -> ./ContactsList 42.27 KB -3.1 KB
../src/components/Tool/Appeal/Flow/DynamicContactFlow.tsx -> ./ContactFlow 64.46 KB -3.1 KB
../src/components/Tool/Appeal/List/ContactsList/DynamicContactsList.tsx -> ./ContactsList 42.27 KB -3.1 KB

Copy link
Contributor

@dr-bizz dr-bizz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking of a way to inform users that the map is using the filters to filter down results, as it is not obvious. I wonder if we should highlight the map icon when we filter, but it might become annoying after some time.

Screenshot 2024-10-23 at 3 43 41 PM

@canac canac requested a review from dr-bizz October 24, 2024 21:30
Copy link
Contributor

@dr-bizz dr-bizz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks great, but more contacts are showing on the map than I expected. I thought only 3 (the same number of the contacts I had on the list view) would show up on maps.

switching-list-to-map.mov

@canac canac requested a review from dr-bizz October 25, 2024 19:05
updateOptions(view as TableViewModeEnum);
const newViewMode = view as TableViewModeEnum;
saveContactsView(newViewMode);
if (newViewMode === TableViewModeEnum.Map && ids.length > 0) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (newViewMode === TableViewModeEnum.Map && ids.length > 0) {
if (newViewMode === TableViewModeEnum.Map && ids.length) {

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Preview Environment Add this label to create an Amplify Preview
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants