-
Notifications
You must be signed in to change notification settings - Fork 56
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
Issue 1221/participant filter #1285
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great in general, but I think the separation of concerns can be improved, as discussed on Zoom just now. Just marking the PR as "changes requested" so that no one else will spend time reviewing it just yet. 😊
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome work! I would only make some minor changes to naming conventions and stuff like that in the code, as commented below. 👇
@@ -10,12 +10,14 @@ import { ZetkinEvent } from 'utils/types/zetkin'; | |||
|
|||
interface EventParticipantsListProps { | |||
data: ZetkinEvent; | |||
filterState: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this (and all related references to the same string) should be called filterString
, not filterState
.
@@ -41,6 +43,7 @@ const ParticipantsPage: PageWithLayout<ParticipantsProps> = ({ | |||
eventId, | |||
orgId, | |||
}) => { | |||
const [filterState, setFilterState] = useState<string>(''); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change the name here as well.
|
||
type ListType = ZetkinEventParticipant | ZetkinEventResponse; | ||
|
||
//TODO filter cancelled list when API supports it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This TODO seems irrelevant here, because there should not need to be any changes to this code when introducing cancelled participants.
type ListType = ZetkinEventParticipant | ZetkinEventResponse; | ||
|
||
//TODO filter cancelled list when API supports it | ||
export function filterList( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would change this line:
export function filterList( | |
export default function filterParticipants( |
And change the name of this module from participantsFilterUtils.ts
to filterParticipants.ts
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for changing the names! This looks good to merge provided that all tests pass. 💯
Description
This PR implements filter with fuzzy search to filter sign up, participants list. It is enable to search input in multiple fields.
Screenshots
Mozilla.Firefox.2023-05-03.14-51-08.mp4
Changes
filterCleared
,filteredParticipants
,filteredSignUp
props toEventParticipantsList
EventParticipantsFilter
Notes to reviewer
I gave a different weight to key first_name for now and didn't write any codes for cancelled list!
Related issues
Resolves #1221