Add option to copy new value to form in edit profile workbench #10885
Conversation
maxidragon
force-pushed
the
copy-changes-to-form
branch
from
9499ab1 to
4b74a32
Compare
There was a problem hiding this comment.
Here are some initial thoughts. Also, please try to open PRs with at least a very basic description of what's happening (as long as the diff is more than ~3 lines).
Screenshots or videos would also be appreciated wherever it makes sense (for example, changing the text of a button does not need a screenshot, but your current PR probably does.)
| useEffect(() => { | ||
| if (defaultValues) { | ||
| setEditedUserDetails((prev) => ({ ...prev, ...defaultValues })); | ||
| } | ||
| }, [defaultValues]); |
There was a problem hiding this comment.
This is not what useEffect was designed for. Is there really no other way?
There was a problem hiding this comment.
I was chatting with Daniel and we came up with this solution - I can try to find a different solution, but there is a high chance that useEffect is necessary here
There was a problem hiding this comment.
One way I can think of where we can avoid useEffect is - pass the editedUserDetails and setEditedUserDetails from the parent component. But since this form is used in two different places, I'm bit reluctant to use that way and will be more inclined towards using useEffect.
There was a problem hiding this comment.
After thinking again, I got one more solution - for this I would like to rename defaultValues to forcedValues. The forcedValues will initially be {}.
When a field is populated, it will become like {name: "John Doe"}.
Now in <Form.Input.../>, we can have the value as {forcedValues?.name || <whatever is already there>} and disabled will be {forcedValues?.name || <whatever is already there>}. This way, if there is a forced value, that will be having highest priority and user won't be able to edit it as well.
While calling the API, instead of sending person as editedUserDetails, we might have to send {...editedUserDetails, ...forcedValues}.
This might be complicated, I'm not sure. @gregorbg WDYT?
There was a problem hiding this comment.
One way I can think of where we can avoid
useEffectis - pass theeditedUserDetailsandsetEditedUserDetailsfrom the parent component. But since this form is used in two different places, I'm bit reluctant to use that way and will be more inclined towards usinguseEffect.
I've also considered this but I think this would make the component much more complicated and harder to use in other places, so I agree with you.
I have no strong preference regarding your second comment, both solutions would probably work but the second one looks more complicated than mine. Let's wait for Gregor's opinion, if you guys think that that would be better, I'm more than happy to implement it in that way.
maxidragon
force-pushed
the
copy-changes-to-form
branch
from
4b74a32 to
02c02cc
Compare
Edited the description, I've uploaded the video |
| @@ -59,6 +70,11 @@ function EditPersonRequestedChangesList({ requestedChanges }) { | |||
| function EditPersonTicketWorkbenchForWrt({ ticketDetails, actingStakeholderId, sync }) { | |||
| const { ticket } = ticketDetails; | |||
| const { save, saving } = useSaveAction(); | |||
| const [defaultValues, setDefaultValues] = useState( | |||
There was a problem hiding this comment.
Do we need to prefill this with default value? Instead if we have it empty and populate whenever the button is clicked, won't this work?
| useEffect(() => { | ||
| if (defaultValues) { | ||
| setEditedUserDetails((prev) => ({ ...prev, ...defaultValues })); | ||
| } | ||
| }, [defaultValues]); |
There was a problem hiding this comment.
One way I can think of where we can avoid useEffect is - pass the editedUserDetails and setEditedUserDetails from the parent component. But since this form is used in two different places, I'm bit reluctant to use that way and will be more inclined towards using useEffect.
| useEffect(() => { | ||
| if (defaultValues) { | ||
| setEditedUserDetails((prev) => ({ ...prev, ...defaultValues })); | ||
| } | ||
| }, [defaultValues]); |
There was a problem hiding this comment.
After thinking again, I got one more solution - for this I would like to rename defaultValues to forcedValues. The forcedValues will initially be {}.
When a field is populated, it will become like {name: "John Doe"}.
Now in <Form.Input.../>, we can have the value as {forcedValues?.name || <whatever is already there>} and disabled will be {forcedValues?.name || <whatever is already there>}. This way, if there is a forced value, that will be having highest priority and user won't be able to edit it as well.
While calling the API, instead of sending person as editedUserDetails, we might have to send {...editedUserDetails, ...forcedValues}.
This might be complicated, I'm not sure. @gregorbg WDYT?
|
Hmm. The problem seems to be that the EditPersonForm holds its own state. That means, it was initially designed to have full control over its own values. |
|
Okay so here's the thing: If you desperately need this and you say WRT is suffocating under the workload unless we get this PR merged in the next 48 hours, then I'll review it in its current state. Otherwise: You need to make clear that
The clean solution is to split these two aspects into two separate components. And then the "inner" component derived from point (2) gets passed the current form state and the Does that idea make sense or would you like some more in-depth explanation? |
|
This is definitely not urgent and can be implemented as you suggested. I'll look into it later and let you know if I need a detailed explanation. |
Screencast.From.2025-02-19.12-54-48.mp4