Dex 761 social group site settings #419

Atticus29 · 2022-07-07T21:20:27Z

Pull request checklist:

Features and bugfixes should be PRed into the develop branch, not main
All text is internationalized
There are no linter errors
- There were some exhaustive-deps and no-unstable-nested-components warnings/errors in DefaultFieldTable.jsx, but the latter were all customBodyRender errors (and I didn't add any more linter errors than already existed on this file). I did a quick spot check, and it doesn't look like we have implemented a fix to this issue yet, but please correct me if I'm wrong.
New features support all states (loading, error, etc)
- N/A

Thanks for keeping it clean! Feel free to merge your own pull request after it has been approved - just use the "squash & merge" option.

QA Notes

Users should be able to add and remove new social group roles.
Users should not be able to save if any roles are empty strings or if there are any duplicates.
Users should be able to designate whether or not a particular role is usable by multiple individuals within a social group.

…ry useState

Emily-Ke

Looks good to me.

Emily-Ke · 2022-07-08T22:43:18Z

src/pages/fieldManagement/settings/DefaultFieldTable.jsx

+  const socialGroups = get(siteSettings, [
+    'social_group_roles',
+    'value',
+  ]);


Should this have a default value of an empty array?

Yes.... fixed this for regions as well. 78f9926.

Emily-Ke · 2022-07-08T22:51:12Z

src/pages/fieldManagement/settings/defaultFieldComponents/SocialGroupsEditor.jsx

+  const emptyRoleLabels = roleLabels.filter(label => !label);
+  if (emptyRoleLabels.length > 0)


This can be simplified with some
https://lodash.com/docs/4.17.15#some or https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/some

Fixed in 78f9926.

Emily-Ke · 2022-07-08T23:07:38Z

src/pages/fieldManagement/settings/defaultFieldComponents/SocialGroupsEditor.jsx

+      <div
+        style={{
+          display: 'flex',
+          justifyContent: 'space-between',
+          alignItems: 'center',
+          width: 500,
+        }}
+      >
+        <Text
+          variant="caption"
+          id="CONFIGURATION_SOCIAL_GROUP_ROLES_DESCRIPTION"
+        />


The wrapper div is unnecessary if you change Text to a block element, for example:

<Text style={{ maxWidth: 500 }} variant="caption" component="p" // "caption" uses a span, so you would have to explicitly specify "p" id="CONFIGURATION_SOCIAL_GROUP_ROLES_DESCRIPTION" />

The Add Role button was shunted to the next line with this change, so I kept it as-it -was.

This is below that one. It is the description, not the label and button.

Ah my bad. Fixed in af9447e.

Emily-Ke · 2022-07-08T23:08:18Z

src/pages/fieldManagement/settings/defaultFieldComponents/SocialGroupsEditor.jsx

+          display: 'flex',
+          justifyContent: 'space-between',
+          alignItems: 'center',
+          width: 500,


maxWidth instead of width will display better on smaller screen sizes.

Fixed in 78f9926

Emily-Ke · 2022-07-08T23:14:23Z

src/pages/fieldManagement/settings/defaultFieldComponents/SocialGroupsEditor.jsx

+          roles,
+          role => (
+            <SocialGroupRole
+              key={role?.guid}


The key cannot be guaranteed to be unique if multiple keys may be set to undefined

For the UI, we should not be including roles that don't exist. These should be filtered before the map.

I think fixed in 78f9926?

Emily-Ke · 2022-07-08T23:18:11Z

src/pages/fieldManagement/settings/defaultFieldComponents/SocialGroupsEditor.jsx

+              onChange={setRoles}
+            />
+          ),
+          [],


#412 (comment)

In my defense, this is not a mistake I have made since re-learning this lesson last time (for maybe the fifth time).

Fixed in 78f9926

Emily-Ke · 2022-07-08T23:30:45Z

src/pages/fieldManagement/settings/defaultFieldComponents/SocialGroupsEditor.jsx

+      {fromErrors && (
+        <Alert severity="error" titleId="AN_ERROR_OCCURRED">
+          {fromErrors.map(error => (
+            <Text key={error} variant="body2">
+              {error}
+            </Text>
+          ))}
+        </Alert>
+      )}


ConfigureDefaultField includes an error display. If you think it is easier to read, I think you can remove this and instead add the map earlier.

<ConfigureDefaultField error={ fromErrors?.map(error => ( <Text key={error} variant="body2"> {error} </Text> ))} // the rest of the props that you already have defined >

Fixed in 78f9926

Emily-Ke · 2022-07-08T23:36:11Z

...es/fieldManagement/settings/defaultFieldComponents/SocialGroupComponents/SocialGroupRole.jsx

+function deleteRole(roles, roleGuid) {
+  const focalRole = find(roles, role => role?.guid === roleGuid);
+  if (focalRole) {
+    const remainingRoles = filter(
+      roles,
+      role => role?.guid !== roleGuid,
+    );
+    return remainingRoles;
+  }
+  return roles;
+}


It would be simpler to just

function deleteRole(roles, roleGuid) { return filter(roles, role => role?.guid !== roleGuid) }

If the roleGuid does not exist, the entire array will still be returned.

Fixed in 78f9926

Emily-Ke · 2022-07-08T23:39:02Z

...es/fieldManagement/settings/defaultFieldComponents/SocialGroupComponents/SocialGroupRole.jsx

+  const checked = get(currentRole, 'multipleInGroup', false);
+
+  return (
+    <div style={{ marginLeft: 32, marginTop: 10 }} key={roleGuid}>


key shouldn't be necessary here.

Fixed in 78f9926

Emily-Ke · 2022-07-08T23:45:56Z

...es/fieldManagement/settings/defaultFieldComponents/SocialGroupComponents/SocialGroupRole.jsx

+  onChange,
+}) {
+  const intl = useIntl();
+  const roleGuid = currentRole?.guid;


If there is no currentRole, should we be returning null or an error message instead?

I just confirmed that the migrated roles will all have guids. And our newly-created ones will all have guids.

If we are only rendering this components on roles that pass the criterion from your other comment:

For the UI, we should not be including roles that don't exist. These should be filtered before the map.
, will this check be necessary?

What if I change what I currently have:
const safeRoles = filter(roles, role => Boolean(role));
to this:
const safeRoles = filter(roles, role => Boolean(role?.guid));?

…lGroupRole component

* Automatic Docker builds on tagged releases and PRs with multi-platform support * Remove nightly action, which was merged into build * Allow all files to be copied in * Dex 761 social group site settings (#419) * DEX-761 flesh out social group form except for multipleInGroup * DEX-761 begin fleshing out the switch * DEX-761 continue with fleshing out the switch * DEX-761 add useEffect * DEX-761 rename the role component, clean up, and remove the unnecessary useState * DEX-761 internationalize and add description as caption * DEX-761 respond to code review feedback * DEX-761 remove unecessary flexbox div * DEX-761 handle the case where a guidless role makes it into the SocialGroupRole component * move server status components to siteStatus directory * Change server status text to site status * Add server status SuperText keys, prevent 'hide' prop from passing to Text * Internationalize aria labels in site status components * dex 1117 - Replace progress with pipeline_status in Asset Group (#420) * Replace asset group progress with pipeline_status * Change px to number and abstract showAlert in AssetGroup * Add public AGS page (#416) * Add public AGS page * Remove unused property * Fixes * Rename things * Remove unused translation key * use new terms (#418) * begin release procedure (#424) * begin release procedure * clarify about merge conflicts * respond to feedback * respond to code review feedback * Update release procedure doc * Run on sentence Co-authored-by: Ben Scheiner <[email protected]> * DEX-922 implement stickyHeader and add a meaningful maxHeight to ever… (#430) * DEX-922 implement stickyHeader and add a meaningful maxHeight to every DataDisplay. Also add horizontalScroll * DEX-922 remove seemingly unnecessary horizontal scroll prop * DEX-922 respond to code review feedback. Remove default maxHeight for Card component and replaced with optional styles, which was then implemented for ImageCard and StatusCard. Remove maxHeight from Card in relationshipCard, remove unused paperStyles from DataDisplay. * DEX-922 respond to code review feedback pt 2 * DEX-922 add tableContainerStyles to DataDisplay and remove maxHeight * DEX-922 remove tableContainerStyles from SightingsDisplay and fix a few little things * DEX-922 final little fixes * Compute new form state from previous state in Settings (#433) * 922 follow up (#435) * DEX-922 print the collaboration data to make the stackBlitz question easier * DEX-922 I do not understand why tableData was undefined, but cool, codex, I can play this game with you * DEX-922 try disablePortal * DEX-922 change the z-index * DEX-922 clean up * DEX-922 respond to code review feedback * Update version to 1.0.2 (#436) Co-authored-by: Jason Parham <[email protected]> Co-authored-by: Mark <[email protected]> Co-authored-by: Ben Scheiner <[email protected]>

Atticus29 added 5 commits July 1, 2022 16:58

DEX-761 flesh out social group form except for multipleInGroup

be38aac

DEX-761 begin fleshing out the switch

1ea312c

DEX-761 continue with fleshing out the switch

3358086

DEX-761 add useEffect

ec61c10

DEX-761 rename the role component, clean up, and remove the unnecessa…

7a54e52

…ry useState

Atticus29 requested review from brmscheiner and Emily-Ke July 7, 2022 21:20

DEX-761 internationalize and add description as caption

c6ce9e6

Atticus29 marked this pull request as ready for review July 7, 2022 23:18

Emily-Ke approved these changes Jul 9, 2022

View reviewed changes

Atticus29 added 4 commits July 11, 2022 11:05

DEX-761 respond to code review feedback

78f9926

DEX-761 remove unecessary flexbox div

af9447e

DEX-761 handle the case where a guidless role makes it into the Socia…

1dc386c

…lGroupRole component

resolve merge conflict in en.json

7b4ce47

Atticus29 merged commit c6f1db1 into develop Jul 11, 2022

Atticus29 deleted the DEX-761-social-group-site-settings branch July 11, 2022 21:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dex 761 social group site settings #419

Dex 761 social group site settings #419

Atticus29 commented Jul 7, 2022 •

edited

Loading

Emily-Ke left a comment

Emily-Ke Jul 8, 2022

Atticus29 Jul 11, 2022 •

edited

Loading

Emily-Ke Jul 8, 2022

Atticus29 Jul 11, 2022

Emily-Ke Jul 8, 2022

Atticus29 Jul 11, 2022

Emily-Ke Jul 11, 2022

Atticus29 Jul 11, 2022

Emily-Ke Jul 8, 2022

Atticus29 Jul 11, 2022

Emily-Ke Jul 8, 2022

Atticus29 Jul 11, 2022

Emily-Ke Jul 8, 2022

Atticus29 Jul 9, 2022

Atticus29 Jul 11, 2022

Emily-Ke Jul 8, 2022

Atticus29 Jul 11, 2022

Emily-Ke Jul 8, 2022

Atticus29 Jul 11, 2022

Emily-Ke Jul 8, 2022

Atticus29 Jul 11, 2022

Emily-Ke Jul 8, 2022

Atticus29 Jul 11, 2022

		const emptyRoleLabels = roleLabels.filter(label => !label);
		if (emptyRoleLabels.length > 0)

Dex 761 social group site settings #419

Dex 761 social group site settings #419

Conversation

Atticus29 commented Jul 7, 2022 • edited Loading

Pull request checklist:

QA Notes

Emily-Ke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Atticus29 Jul 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Atticus29 commented Jul 7, 2022 •

edited

Loading

Atticus29 Jul 11, 2022 •

edited

Loading