-
Notifications
You must be signed in to change notification settings - Fork 46
[Issue #5931] Hide save button when application is submitted #6870
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
base: main
Are you sure you want to change the base?
Conversation
| async function FormPage({ params }: formPageProps) { | ||
| const { applicationId, appFormId } = await params; | ||
| const { data, error } = await getFormData({ applicationId, appFormId }); | ||
| let details = {} as ApplicationDetailsCardProps; |
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 block is copied from the top-level application page, using the same logic to fetch the application information
doug-s-nava
left a comment
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.
works correctly! a couple minor requests and things to think about
.github/CODEOWNERS
Outdated
| * @mdragon # project lead | ||
|
|
||
| /documentation/ @acouch @andycochran @btabaska @chouinar @doug-s-nava @joshtonava @mdragon @myduong-navapbc @widal001 @prasnava @jakobpederson @ErinPattisonNava @glenblosser-nava # everyone | ||
| /documentation/ @acouch @andycochran @btabaska @chouinar @doug-s-nava @joshtonava @mdragon @myduong-navapbc @widal001 @prasnava @jakobpederson @ErinPattisonNava @glenblosser-nava @brianlee-nava # everyone |
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.
github is saying that @brianlee-nava may not exist or have access to the project. Certainly seems like the user exists, can we confirm access?
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'll double check. He asked me to add him to the codeowners list since I was doing a PR anyway
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.
also wanted to make sure that @brianlee-nava knows what he's signing up for here - any time a PR comes out with an update to documentation he'll be pinged with an email for a review. I'm all for that if he wants to contribute that way, but since none of the other PMs are in the list, wanted to check
...[locale]/(base)/workspace/applications/application/[applicationId]/form/[appFormId]/page.tsx
Show resolved
Hide resolved
doug-s-nava
left a comment
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.
a few notes on approach for read only widgets, happy to talk it through
| name={id} | ||
| required={required} | ||
| disabled={disabled} | ||
| disabled={disabled || readonly} |
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 approach means each widget needs to implement disabling fields based on the "readonly" prop individually. Since each field (except in the budget form) is rendered through the "renderWidget" function, and each widget takes a "disabled" prop, we could do this calculation there instead, and pass "disabled: true" from "renderWidget" into each widget whenever "readonly" is true. We'd still have to figure out the budget form, but this should handle every other widget type without touching the individual widgets themselves
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 approach was copied from our other widgets, I can change it if need be. I've gone ahead and had formFields pass both readonly and disabled to its children to simplify future implementation
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.
not sure what you mean about copying from other widgets. My concern is that this way each widget needs to interpret readonly separately when we could potentially isolate that logic in the widget renderer. Maybe we can get on a call to show you what I'm thinking?
doug-s-nava
left a comment
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.
Want to make sure we're on the same page on where to calculate the disabled props before I approve, but this all works! nice job.
.github/CODEOWNERS
Outdated
| * @mdragon # project lead | ||
|
|
||
| /documentation/ @acouch @andycochran @btabaska @chouinar @doug-s-nava @joshtonava @mdragon @myduong-navapbc @widal001 @prasnava @jakobpederson @ErinPattisonNava @glenblosser-nava # everyone | ||
| /documentation/ @acouch @andycochran @btabaska @chouinar @doug-s-nava @joshtonava @mdragon @myduong-navapbc @widal001 @prasnava @jakobpederson @ErinPattisonNava @glenblosser-nava @brianlee-nava # everyone |
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.
also wanted to make sure that @brianlee-nava knows what he's signing up for here - any time a PR comes out with an update to documentation he'll be pinged with an email for a review. I'm all for that if he wants to contribute that way, but since none of the other PMs are in the list, wanted to check
| schema: RJSFSchema; | ||
| uiSchema: UiSchema; | ||
| formContext?: RootBudgetFormContext; | ||
| readonly?: boolean; |
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.
sry to not have mentioned this earlier - can we change this to readOnly for camelCase consistency?
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.
Got it
| name={id} | ||
| required={required} | ||
| disabled={disabled} | ||
| disabled={disabled || readonly} |
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.
not sure what you mean about copying from other widgets. My concern is that this way each widget needs to interpret readonly separately when we could potentially isolate that logic in the widget renderer. Maybe we can get on a call to show you what I'm thinking?
| console.error(`Unknown widget type: ${type}`, definition); | ||
| throw new Error(`Unknown widget type: ${type}`); | ||
| } | ||
| if (props.isFormLocked) { |
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 making this change - with this in place I think you can roll back the readOnly changes to each of the individual widgets other than the ones that weren't already handling disabled field states (which I think is the attachment widgets and budget form widgets)
|
split "readonly / disabled" into its own ticket #7067 |
## Summary Work for #5931 ## Changes proposed When the form is not in progress, the Save button is hidden This PR includes work that will be implemented in a future PR for #5931 ## Context for reviewers This PR is part of larger work on the #5931 ticket, but _only_ features the hidden save button change. The readonly changes are also present in [this](#6870) PR. ## Validation steps After submitting an application, the Save button should not be visible on forms. --------- Co-authored-by: Glen Blosser <[email protected]>
|
@glenblosser-nava can we close this PR at this point or are you going to come back to it for the disabled fields? |
Summary
Work for #5931
Changes proposed
Adds conditional rendering to the save button, reliant on the application's status not being submitted or accepted
Context for reviewers
#5931
Validation steps
Submit an application in a dev environment, and verify that the save button disappears