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

Use a Context for fetching Registrations #10323

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

Conversation

FinnIckler
Copy link
Member

From my basic testing everything seems to work as expected. Would like someone more confident in Context design weigh in if stuff like

const isRegistered = Boolean(registration) && registration.competing.registration_status !== 'cancelled';
const isAccepted = isRegistered && registration.competing.registration_status === 'accepted';
const isRejected = isRegistered && registration.competing.registration_status === 'rejected';

makes sense to expose in the Context or not

Copy link
Contributor

@kr-matthews kr-matthews left a comment

Choose a reason for hiding this comment

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

Would like someone more confident in Context design weigh in if stuff like

const isRegistered = Boolean(registration) && registration.competing.registration_status !== 'cancelled';
const isAccepted = isRegistered && registration.competing.registration_status === 'accepted';
const isRejected = isRegistered && registration.competing.registration_status === 'rejected';

makes sense to expose in the Context or not

Makes sense to me 👍

@FinnIckler
Copy link
Member Author

This works, but now isProcessing: pollingStatus !== 'success' || pollingData.processing feels a bit weird to me because isProcessing will be true before we start polling (you have to do this otherwise isProcessing will be false during the fetching of the data). Maybe processing should be a string with 'pending', 'processing' and 'processed' instead?

@kr-matthews
Copy link
Contributor

This works, but now isProcessing: pollingStatus !== 'success' || pollingData.processing feels a bit weird to me because isProcessing will be true before we start polling (you have to do this otherwise isProcessing will be false during the fetching of the data). Maybe processing should be a string with 'pending', 'processing' and 'processed' instead?

Hm I thought isPolling would be only inside the provider, and not shared via the hook (only isProcessing and startPolling provided). The point of polling is to figure out whether you're done processing right? Components should only care whether the registration is processing, not how we're figuring that out (i.e. with polling). Although I guess that's already broken if they have access to startPolling...

I feel like maybe the provider should be responsible for doing the registration actions too, e.g. refetchRegistration that CompetingStep is currently calling. Then CompetingStep doesn't have to observe isProcessing and isPolling, and can just observe registration or some other property and see directly from that when to call nextStep() (I think? I'm not familiar with the backend/processing part of registrations, so maybe I don't know what I'm talking about).

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.

2 participants