x1032 Add user full name to Release Recipient #393
Conversation
There was a problem hiding this comment.
I have seen the following issues
- The layout of columns are not correct
- The recipient names are not saved or updated
Hi @seenanair, thank you for reviewing this
- I fixed the layout in the second commit, is it still not right at your end ?
- The user should only be able the edit the recipient user full name, unless you mean the user full name ? is there any errors ?
| username, | ||
| enabled | ||
| })); | ||
| }; |
There was a problem hiding this comment.
Why this conversion is required?
Also, It will be better to define this in a global file and not in EntityManager.
There was a problem hiding this comment.
As the userFullName field is optional, the conversion is only to set it to empty if undefined
There was a problem hiding this comment.
Ok, is it for displaying purpose? If so, is it not possible to check it on the fly and display an empty string?
| <TableCell colSpan={2}> | ||
| <Input | ||
| type="text" | ||
| placeholder="Enter user full name" |
There was a problem hiding this comment.
This is generic class, so I think you should avoid any specifics here. For e.g, if we need to add an extra column in another config tab, this won't make sense
| <TableCell colSpan={2}>{entity[displayKeyColumnName]}</TableCell> | ||
| {extraDisplayColumnName && ( | ||
| <TableCell colSpan={2}> | ||
| <Input |
There was a problem hiding this comment.
Would it be better to switch it back to a label or input style resembling a label ,once user finishes editing or on blur?
| <TableCell colSpan={2}> | ||
| <Input | ||
| type="text" | ||
| placeholder="Enter user ID" |
There was a problem hiding this comment.
We should avoid messages specific to a particular context as this is a generic class.
| type BasicReleaseRecipient = { | ||
| userFullName: string; | ||
| username: string; | ||
| enabled: boolean; |
There was a problem hiding this comment.
Why this need to be redefined as we have already a graphql fragment - ReleaseRecipientFieldsFragment available?
src/pages/Configuration.tsx
Outdated
| @@ -347,7 +347,7 @@ export default function Configuration({ configuration }: ConfigurationParams) { | |||
| also available on the <StyledLink to={'/sgp'}>SGP Management</StyledLink> page as "Work Requester" | |||
| </p> | |||
| <EntityManager | |||
| initialEntities={configuration.releaseRecipients} | |||
| initialEntities={convertToBasicReleaseRecipient(configuration.releaseRecipients)} | |||
There was a problem hiding this comment.
Why we need these conversions?
There was a problem hiding this comment.
Same as obove, as the userFullName is defined as optional
|
|
src/pages/Configuration.tsx
Outdated
| .then((res) => convertToBasicReleaseRecipient(res.updateReleaseRecipientFullName)[0]); | ||
| label: 'Full Name', | ||
| value: 'fullName', | ||
| extraFieldPlaceholder: 'Enter User Full Name', |
There was a problem hiding this comment.
Probably you can change this as well to 'Enter full name' to match column header
| @@ -290,7 +272,7 @@ export default function EntityManager<E extends Record<string, EntityValueType>> | |||
| <TableCell colSpan={2}> | |||
| <Input | |||
There was a problem hiding this comment.
Can we have the class style for the full name to be as no border (or a very subtle one) as in normal condition and when we start editing (that means when it gets focus), the border can appear? This way the field will look more similar to other, I think
No description provided.