Opt-In / Opt-Out program flow#34
Conversation
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
| <Loading /> | ||
| {/if} | ||
|
|
||
| <Modal |
There was a problem hiding this comment.
Let's use a PanelFormDialog here, it will behave a lot better long term since we don't know what downstream might want to do with their prompt. Also much easier to wire up than this was.
|
|
||
| <Modal | ||
| bind:open | ||
| modalHeading={`${optIn ? 'Opt in to' : 'Opt out of'} ${prompt?.title}?`} |
There was a problem hiding this comment.
Shouldn't prompt?.title be the program/application title?
| on:close={() => { prompt = {} }} | ||
| class='opt-out-modal' | ||
| > | ||
| {#key prompt.id} |
There was a problem hiding this comment.
This {#key} thing was specific to how the prompt screen was working. You won't need it here.
| moot: 0 | 1 | ||
| locked: 0 | 1 | ||
| invalidated: 0 | 1 | ||
| optOut: 0 | 1 |
There was a problem hiding this comment.
Lots of API-side code hanging around that shouldn't be necessary. More details in other comments.
| }), {} as Record<string, string | undefined>) | ||
|
|
||
| $: optOutPrograms = applications.reduce((acc, curr) => { | ||
| const optOut = curr.requirements.flat().flatMap(r => r.prompts).find(r => r.optOut) |
There was a problem hiding this comment.
It looks like you're depending on the API for which prompt is an optOut prompt and ignoring the optOut in the UI's registry. I suggest:
const optOut = curr.requirements.flat().flatMap(r => r.prompts).find(p => uiRegistry.getPrompt(p.key)?.optOut)| > | ||
| {#key prompt.id} | ||
| <Form bind:store hideFallbackMessage unsavedWarning submit={onSubmit} validate={onValidate} preloadAsDraft={!prompt.hasSavedData} preload={prompt.preloadData} let:data> | ||
| <svelte:component this={def!.formComponent} {data} appRequestId={appRequest.id} appRequestData={appRequest.data} fetched={prompt.fetchedData} configData={prompt.configurationData} gatheredConfigData={prompt.gatheredConfigData} store={store} /> |
There was a problem hiding this comment.
I see why you did it but I wouldn't add the store here without adding it to the other places that render prompt components. We have to present a consistent set of props to the downstream.
I probably wouldn't add it, as it's already in context. You can do this inside a prompt:
import { FORM_CONTEXT, type FormStore } from '@txstate-mws/svelte-forms'
const store = getContext<FormStore>(FORM_CONTEXT)| }, {} as Record<string, OptOutApplication | undefined>) | ||
|
|
||
| $: optedOutPrograms = applications.filter(curr => curr.requirements.flat().flatMap(r => r.prompts).find(r => r.optOut)).reduce((acc, c) => { | ||
| const optOutRequirement = c.requirements.flat().find(r => r.prompts.find(p => p.optOut)) |
There was a problem hiding this comment.
Same dependence on API here. Also r.prompts.some is a little more clear than r.prompts.find. Suggest:
const optOutRequirement = c.requirements.flat().find(r => r.prompts.some(p => uiRegistry.getPrompt(p.key)?.optOut))| description: 'Opt Out', | ||
| schema: OptOutSchema, | ||
| optOut: true, | ||
| prestage: () => ({ |
There was a problem hiding this comment.
oof, I see why you did this and it's going to be confusing for folks on the regular, but I think the requirement just needs to treat undefined as MET instead of trying to inject data into here.
| resolve: (data, config) => { | ||
| const promptData = data['opt_out_prompt'] as OptOutData | ||
| if (promptData?.optOut) return { status: RequirementStatus.DISQUALIFYING } | ||
| if (promptData?.optOut == null) return { status: RequirementStatus.PENDING } |
There was a problem hiding this comment.
This is what I was referring to in the earlier comment. Don't return PENDING here, just return MET or NOT_APPLICABLE if they have never opened the modal.
| prestage: () => ({ | ||
| optOut: false | ||
| }), | ||
| validate: (data, config) => { |
There was a problem hiding this comment.
I feel like this validate could be omitted once the requirement is updated to accept undefined as MET. No big deal to include it but I want to be sure it's not needed because that'd be an ugly thing for downstream to have to remember to do.
2 participants
txstate-etc/reqquest-txstate#267