Refactor Actions List Item #229

willm30 · 2023-01-26T17:47:06Z

Description

This PR refactors the ActionsListItem to:

a) Use a base ListItem component that can be shared with DecisionsListItem
b) Handle all the title-related logic outside the component.

Testing

More of a straight-forward code review, just make sure the ActionListItem works the same as in #231.

New stuff ✨

ListItem
Mapping between action types and message descriptor values in en-actions

Changes 🏗

Combined SetUserRoles in en-actions and updated formatRolesTitle utility
Minor tidying of InputLabel
Moved ActionsListItem to ColonyActions since it's no longer a shared component.

Contributes to #207

willm30 · 2023-01-31T11:27:16Z

src/Entry.tsx

@@ -41,7 +40,6 @@ const Entry = ({ store }: Props) => {
        ...actionMessages,
        ...eventsMessages,
        ...systemMessages,
-        ...motionMessages,


We seem to have moved away from having two separate files for actions and motions messages, which makes sense since they share the same set of messages, so have removed.

willm30 · 2023-01-31T11:29:07Z

src/components/common/ColonyActions/ActionsListItem/ActionsListItem.tsx

+  item: FormattedAction;
+}
+
+const ActionsListItem = ({


There are two parts to this refactor. First is the abstraction of the view ListItem, which will also be shared by DecisionListItem.

The second is handling the logic, most of which was dedicated to formatting the title correctly. This is now handled by getActionListItemTitleValues.

The commented out sections refer to motions functionality.

willm30 · 2023-01-31T11:31:26Z

src/components/common/ColonyActions/ActionsListItem/config/config.ts

+};
+
+/* Returns the correct message values according to the action type. */
+const getActionListItemTitleValues = (


Before we were passing the FormattedMessage component a massive object with all possible title values. Now we fetch the relevant values, according to the action type, and only pass in those.

All this needs to be maintained is that the mapping above gets updated every time a new message is added at en-actions.

willm30 · 2023-01-31T11:34:41Z

src/components/common/ColonyActions/ActionsListItem/config/helpers.tsx

@@ -0,0 +1,146 @@
+import React from 'react';


I can see this file being merged with config.ts in the event the data wrangling requirements are simplified (e.g. no async) and we don't have much to handle in the component. But for now I've separated as it makes it easier to see what's going on in config.

This file is basically just responsible for ensuring that the items object has key value pairs that match what we're expecting in our message descriptors.

willm30 · 2023-01-31T11:36:50Z

src/components/common/ColonyActions/ActionsListItem/config/helpers.tsx

+
+  /*
+
+Note that the following transformations also exist in the Dapp. Because they are async,


I've left this comment here as a reminder of outstanding transformations that hopefully will be handled by the cdapp equivalent of the actions resolver.

Can't do anything in this pr since we're using mock data.

Local resolvers are a deprecated feature, so custom hooks sound like the best next option

@jakubcolony Or have all/most of the transformation handled by the block ingestor? I.e. it picks up the on-chain event and adds to the db an actions item that's as close as possible (minus the jsx) to the format expected by the actions list item. (E.g. providing a User object instead of an address string). Then we query the db for that action to display in the list.

Isn't that more or less the flow we're expecting to implement anyway?
cdapp => on-chain event => block ingestor => db => cdapp

That's kinda the general idea of the block investor anyway, to handle as much data processing as possible, so we can offload it from the client.

Granted, this is not possible in all cases, but where that's the case, we should strive to achieve that.

willm30 · 2023-01-31T11:41:14Z

src/components/shared/ListItem/ListItem.tsx

+      >
+        <div
+          className={styles.avatar}
+          onClick={(e) => e.stopPropagation()}


So when you click on the avatar it doesn't fire the parent div's click event

willm30 · 2023-01-31T11:41:43Z

src/hooks/useFormatRolesTitle.ts

@@ -1,80 +0,0 @@
-import { isEmpty } from 'lodash';


This doesn't need to be a hook any longer so moved to ~utils/colonyActions

willm30 · 2023-01-31T11:42:28Z

src/i18n/en-actions.ts

      other {Generic action we don't have information about}
    }`,
-  [`action.${ColonyActions.SetUserRoles}.assign`]: `Assign the {roles} in {fromDomain} to {recipient}`,


Simplified these. Exact text is handled by formatRolesTitle in ~utils/colonyActions

willm30 · 2023-01-31T11:43:06Z

src/i18n/en-actions.ts

  'action.type': `{actionType, select,
      ${ColonyActions.WrongColony} {Not part of the Colony}
      ${ColonyActions.Payment} {Payment}
+      ${ColonyMotions.PaymentMotion} {Payment}


Added motions here since they've already been added to action.title (which I think is a good idea since actions and motions share the same messages)

willm30 · 2023-01-31T11:44:26Z

src/utils/intl.ts

@@ -51,6 +50,26 @@ const isMessageDescriptor = (message?: Message): message is MessageDescriptor =>

 const { formatMessage: formatIntlMessage } = intl<ReactNode>();

+const addKeyToFormattedMessage = (


Required when complex message values are included in a component that is part of a list (e.g. actions list item)

jakubcolony

LGTM, great idea with abstracting this!

jakubcolony · 2023-01-31T20:19:21Z

src/components/common/ColonyActions/ActionsListItem/ActionsListItemMeta.tsx

+  commentCount: any;
+  fromDomainId: any;


Could better types be provided here?

jakubcolony · 2023-01-31T20:25:15Z

src/components/common/ColonyActions/ActionsListItem/config/config.ts

+    (values, key) => {
+      // eslint-disable-next-line no-param-reassign
+      values[key] = updatedItem[key];
+      return values;


Could this be rewritten so that it doesn't mutate the values (meaning we can get rid of the rule disable)?
For example:

return { ...values, [key]: updatedItem[key] }

jakubcolony · 2023-01-31T20:27:25Z

src/components/common/ColonyActions/ActionsListItem/config/helpers.tsx

+
+  /*
+
+Note that the following transformations also exist in the Dapp. Because they are async,


Local resolvers are a deprecated feature, so custom hooks sound like the best next option

rdig

Very nice work again.

Left some comments, one of which requires attention, but other than that, it's good to go.

rdig · 2023-02-01T13:09:11Z

src/components/common/ColonyActions/ActionsListItem/ActionsListItemMeta.tsx

+  titleCommentCount: {
+    id: `${displayName}.titleCommentCount`,
+    defaultMessage: `{formattedCommentCount} {commentCount, plural,
+        one {comment}
+        other {comments}
+      }`,
+  },


We won't have comments enabled for the initial launch.

So either remove these, or just comment them out

rdig · 2023-02-01T13:14:07Z

src/components/common/ColonyActions/ActionsListItem/config/helpers.tsx

+
+  /*
+
+Note that the following transformations also exist in the Dapp. Because they are async,


That's kinda the general idea of the block investor anyway, to handle as much data processing as possible, so we can offload it from the client.

Granted, this is not possible in all cases, but where that's the case, we should strive to achieve that.

rdig · 2023-02-01T13:19:08Z

src/components/shared/ListItem/ListItem.tsx

+      >
+        <div
+          className={styles.avatar}
+          onClick={(e) => e.stopPropagation()}


Shouldn't this be handled inside a callback ?

This is because, having it like this, will instantiate 2 new anonymous functions on every render and subsequent re-renders.

Having it as a callback instantiates it just once.

jakubcolony · 2023-02-01T22:06:57Z

src/types/actions.ts

+export type ActionUserRoles = {
+  id: ColonyRole;
+  setTo: boolean;
+};
+


Could you clean up the imports in this file? They still contain the old ~core or ~data

willm30 self-assigned this Jan 26, 2023

willm30 force-pushed the port/207-actions-list-item branch from 2a88811 to 4375158 Compare January 30, 2023 14:48

willm30 changed the base branch from port/colony-actions to port/207-actions-list January 30, 2023 14:48

willm30 force-pushed the port/207-actions-list-item branch from 4375158 to 4a22db9 Compare January 30, 2023 14:49

rdig mentioned this pull request Jan 30, 2023

Port Actions List #231

Merged

3 tasks

willm30 force-pushed the port/207-actions-list-item branch from 4a22db9 to 8a43f70 Compare January 30, 2023 15:03

willm30 force-pushed the port/207-actions-list branch 3 times, most recently from 14f4ce3 to 561ffe1 Compare January 30, 2023 17:54

willm30 changed the title ~~Port Actions List~~ Refactor Actions List Item Jan 30, 2023

willm30 force-pushed the port/207-actions-list branch from 561ffe1 to baa6433 Compare January 30, 2023 17:58

willm30 force-pushed the port/207-actions-list-item branch 2 times, most recently from 88bd902 to 159fc62 Compare January 30, 2023 18:03

willm30 force-pushed the port/207-actions-list branch 5 times, most recently from 2010a41 to 0f7853b Compare January 31, 2023 11:01

willm30 force-pushed the port/207-actions-list-item branch from 159fc62 to 4c3227c Compare January 31, 2023 11:12

willm30 force-pushed the port/207-actions-list branch 2 times, most recently from 22900e4 to cf77ae5 Compare January 31, 2023 11:22

Refactor: Action List Item

2c180f1

willm30 force-pushed the port/207-actions-list-item branch from 6d714d9 to df68f6f Compare January 31, 2023 11:26

willm30 commented Jan 31, 2023

View reviewed changes

willm30 linked an issue Jan 31, 2023 that may be closed by this pull request

Port over Actions List component #207

Closed

willm30 marked this pull request as ready for review January 31, 2023 12:11

willm30 requested a review from a team January 31, 2023 12:11

willm30 mentioned this pull request Jan 31, 2023

Port draft decision list item #201

Merged

jakubcolony approved these changes Jan 31, 2023

View reviewed changes

rdig approved these changes Feb 1, 2023

View reviewed changes

jakubcolony reviewed Feb 1, 2023

View reviewed changes

Misc: tidy

a2e8912

willm30 force-pushed the port/207-actions-list-item branch from df68f6f to a2e8912 Compare February 2, 2023 14:58

willm30 merged commit dc3cef0 into port/207-actions-list Feb 2, 2023

willm30 deleted the port/207-actions-list-item branch February 2, 2023 15:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor Actions List Item #229

Refactor Actions List Item #229

willm30 commented Jan 26, 2023 •

edited

willm30 Jan 31, 2023

willm30 Jan 31, 2023

willm30 Jan 31, 2023

willm30 Jan 31, 2023

willm30 Jan 31, 2023

jakubcolony Jan 31, 2023

willm30 Feb 1, 2023

rdig Feb 1, 2023

willm30 Jan 31, 2023

willm30 Jan 31, 2023

willm30 Jan 31, 2023

willm30 Jan 31, 2023

willm30 Jan 31, 2023

jakubcolony left a comment

jakubcolony Jan 31, 2023

jakubcolony Jan 31, 2023

jakubcolony Jan 31, 2023

rdig left a comment

rdig Feb 1, 2023

rdig Feb 1, 2023

rdig Feb 1, 2023

jakubcolony Feb 1, 2023


		/*

		Note that the following transformations also exist in the Dapp. Because they are async,

		@@ -51,6 +50,26 @@ const isMessageDescriptor = (message?: Message): message is MessageDescriptor =>

		const { formatMessage: formatIntlMessage } = intl<ReactNode>();

		const addKeyToFormattedMessage = (

Refactor Actions List Item #229

Refactor Actions List Item #229

Conversation

willm30 commented Jan 26, 2023 • edited

Description

Testing

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

jakubcolony left a 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

rdig left a 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

willm30 commented Jan 26, 2023 •

edited