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

Add email option to members identities filter list #1478

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

Conversation

peoray
Copy link
Contributor

@peoray peoray commented Sep 12, 2023

Changes proposed ✍️

What

Fixes #703

🤖 Generated by Copilot at 57c829d

This pull request adds email filtering and searching to the member module in the frontend. It updates the identities filter config and the placeholder text of the main filter in frontend/src/modules/member/config/filters.

🤖 Generated by Copilot at 57c829d

Search by name or email
A new filter option
Spring cleaning the frontend

Why

How

🤖 Generated by Copilot at 57c829d

  • Add a new option to filter members by email address in the identities filter (link)
  • Update the placeholder text of the member search filter to include email (link)

Checklist ✅

  • Label appropriately with Feature, Improvement, or Bug.
  • Add screehshots to the PR description for relevant FE changes
  • New backend functionality has been unit-tested.
  • API documentation has been updated (if necessary) (see docs on API documentation).
  • Quality standards are met.

@joanagmaia
Copy link
Contributor

Hey @peoray, if you need any further help on this one let me know 😄

@peoray
Copy link
Contributor Author

peoray commented Sep 20, 2023

@joanagmaia I do. I left a comment on the issue itself :)

@peoray
Copy link
Contributor Author

peoray commented Sep 26, 2023

@joanagmaia how can I make sure Email is added as one of the select options?
I'm getting an unknown field operator error
Unknown field or operator: email!

@joanagmaia
Copy link
Contributor

@joanagmaia how can I make sure Email is added as one of the select options? I'm getting an unknown field operator error Unknown field or operator: email!

You cannot place email inside identities. You need to treat them almost as separate filters.

  • So if it's a platform you use what we already have in apiFilterRenderer.
  • If it's email, you need to parse with customized logic, and you need to use emails key instead of email. You can check how we do for search bar in modules/member/config/filters/main.ts
  • If it's both you probably need to have an or operator to treat them both differently

@peoray
Copy link
Contributor Author

peoray commented Oct 9, 2023

@joanagmaia to iterate:

  • When only platforms are selected
  • when only email is selected
  • when both platforms and email are selected

I have updated the code with the current work progress, but it still doesn't work as expected.

One more thing, for the first option, if multiple options are selected, for example, Twitter and DEV, what's the expected result?

@peoray
Copy link
Contributor Author

peoray commented Oct 15, 2023

@joanagmaia Any update on this? Would like to get it over the finish line 🙏🏿

@joanagmaia
Copy link
Contributor

@joanagmaia Any update on this? Would like to get it over the finish line 🙏🏿

Hey @peoray, sorry for the delay! I'll look into the code and your comment and check what's the issue. I'll make an effort to reply to you today

@joanagmaia
Copy link
Contributor

Hey @peoray, just took a look at your comments and functionality-wise wise it is looking good to me.
When multiple platforms are selected, we should display members that have at least one of the selected platforms

I'm gonna review the code as well to make sure it is good to go 😄

@peoray peoray marked this pull request as ready for review October 19, 2023 20:09
Copy link
Contributor

@joanagmaia joanagmaia left a comment

Choose a reason for hiding this comment

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

It looks good to me 😄 Could you just fix linting?

@joanagmaia
Copy link
Contributor

Hey @peoray, not sure if you missed on my message, but everything looks ok with PR, just missing to fix lint issues. You can run npm run lint to check them out

@peoray
Copy link
Contributor Author

peoray commented Oct 26, 2023

@joanagmaia good to go :)

@peoray
Copy link
Contributor Author

peoray commented Nov 20, 2023

@joanagmaia what's the status on this :)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[C-1048] Members: Search members with associated email
2 participants