dex 1341 - add collaboration from user page #464
Conversation
| @@ -0,0 +1,31 @@ | |||
| import { useIntl } from 'react-intl'; | |||
There was a problem hiding this comment.
I feel like I have been told not to import react libraries into files that aren't react components. Did the linter yell about this?
I think the pattern elsewhere might involve passing intl as an argument into the (in this case) useHandleRequestError method.
There was a problem hiding this comment.
I feel like I have been told not to import react libraries into files that aren't react components. Did the linter yell about this?
No, because this isn't a vanilla utility function. It is a custom hook.
There was a problem hiding this comment.
I'm not sure if it makes more sense as a custom hook or a utility function, so I just left it as a hook for now.
| async function mutationFn({ userGuid: secondUserGuid }) { | ||
| try { | ||
| const result = await axios.request({ | ||
| url: prefixApiUrl('/collaborations/'), |
There was a problem hiding this comment.
this method name makes it sound like it's gonna turn whatever input into a prefixApiUrl... perhaps rename to appendToPrefixUrl or something like that?
There was a problem hiding this comment.
Renamed to withApiPrefix in b8110f0. Hopefully that name makes more sense.
| setSelectedUserGuid(null); | ||
| } | ||
|
|
||
| function handleSubmitCollaborator() { |
There was a problem hiding this comment.
This seems like maybe an unnecessary wrapper method? Perhaps pass an anonymous () => addCollaboration(
{ userGuid: selectedUserGuid },
{ onSuccess: handleCloseDialog },
)
To the button's onClick prop instead?
| }} | ||
| > | ||
| {users && ( | ||
| <form |
There was a problem hiding this comment.
I'm curious why a normal form element here.
There was a problem hiding this comment.
What would you use instead?
There was a problem hiding this comment.
I don't know. I see FormControl more often in the code base. Mostly I was hoping to learn whether it was a conscious choice to not use FormControl and why.
There was a problem hiding this comment.
FormControls are wrappers that provide functionality for inputs. The TextField that I am using in this form "is a complete form control". The form could have multiple FormControls, one for each input. I did intentionally use a form because I think it is semantically correct and because it provides some nice behavior by default.
https://v4.mui.com/components/text-fields/#textfield
I picked TextField because that is what was used in IndividualSelector.
Pull request checklist:
developbranch, notmainWhich JIRA ticket(s) and/or GitHub issues does this PR address?
Pull Request Overview
no-unstable-nested-componentslint error in CollaborationsCardReview notes
viewerIsUserManagerto UserProfile is the right choice. It seemed to simplify some of the logic, but at the expense of a more complicated API.