-
Notifications
You must be signed in to change notification settings - Fork 55
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 1542/refactor EVENTS into hooks #1585
Issue 1542/refactor EVENTS into hooks #1585
Conversation
…move old LocationsModel
…r on hooks and implement changes in components
…missing components
…ypes definitions ZetkinEventPatchBody and ZetkinEventPostBody
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.
Amazing work on this major refactor! I think the code looks mostly great. I did find some opportunities for optimization (loading only necessary events instead of loading all of them) and also some design decisions that I don't fully agree with.
I especially think we need as a team to talk about:
- Should mutation hooks take the ID of the object they're meant to mutate in the hook, or in the mutation functions? (I prefer the former)
- Should hooks that load data generally return a future or an object containing a future (and if so, should it be called e.g.
eventFuture
or justevent
)? - Where do we put "create" actions, i.e. mutations that create a new object on the server? They don't quite fit in the normal mutation hooks (which are for a single, already existing object) nor in the plural hooks, because we don't want to retrieve data before creating. Maybe a separate hook, e.g.
useCreateEventType()
etc?
…nstead of useAllEvents
…hook in useRelatedEvents
…atus to set participant status and inplement changes
…n't using route query before
…unnecessary eventId
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 on this massive PR! It's very big, so I might have missed something, but the code looks good to me. I haven't tested it, but if it's working as expected, I think it can be merged.
a2cb85e
into
issue-1542/refactor-to-use-hooks
Description
This PR refactors all Event Models into hooks
Changes
EventDataModel
,EventTypesModel
,EventsModel
andLocationsModel
ZUISpeedDial
and /TaskList
useAllEvents
(will be removed when mergin Campaigns hook)useDuplicateEvent
useEventContact
useEvent
anduseEventMutations
useEventLocations
anduseEventLocationsMutations
useEventParticipants
anduseEventParticipantsMutations
useEventState
useEventTypes
useEventsMutations
useParallelEvents
useParticipantsStatus
useRelatedEvents
useCreateType
useCreateEvent
Notes to reviewer
Repo file will be removed in another PR.
useAllEvents
hook is used only byAllCampaignsSummaryPage
and it will be removed after merge Campaigns refactor PR #1586Related issues
Part of issue 1542