refactor(ui): WorkflowsToolbar component from class to functional #12046

juijeong8324 · 2023-10-20T13:04:35Z

Partial Fixes for #9810

Motivation

Use newer, modern functional components compatible with hooks instead of legacy class-based components

Modifications

class fo functional

this.props -> arguments
methods -> inner functions

Delete loadWorkflows() from WorkflowsToolBar props

Beacause ListWatch() update workflows when workflows was performed action

getNumberSelected() -> WorkflowsToolbar's local variable numberSelected

This function get the number of selected workflows from calling props. However it gets called multiple times each rendering.
So, when they are updated, we can get the number of selected workflows from numberSelected once without multiple function call.

using popup and notifications with useContext

delete renderActions

move rendering Action buttons element toWorkflowsToolBars rendering
local variable
- actionButtons -> delete
- actions -> local variable in useMemo
- disabled -> local variable in useMemo
- groupActions -> variable groupAction in WorkflowsToolbar
  - using useMemo with dependency props.selectedWorkflows

modify name in WorkflowsGroupAction and variable

interface WorkflowsGroupAction -> WorkflowsOperation
- Beacuse I think it is ambiguous what 'group' means 'workflows' or 'Action'
- WorkflowsOperation is similar with WorkflowOperation
groupIsDisabled -> isDisabled
- I think 'disabled' is more related with "operation" not "group",
- Also, isDiabled -> actionIsDiabled in WorkflowsToolBarProps
className -> delete
- It is same action.title
groupAction -> workflowsAction

modify function performActionOnSelectedWorfklows

async/await -> delete
- await keyword use in popup that check action
- popup move to workflowsAction
This function is called only when action are performed in the selected workflow.

Verification

Pure refactor, no real semantic changes
Tested in Local environment

Screenshot below, Action of delete workflows
Screenshot below, Action of resubmit workflows
Screenshot below, action button when workflows running

Signed-off-by: juijeong8324 <juijeong8324@gmail.com>

agilgur5

Thanks for diving into this and your detailed explanations!

The renames and re-ordering took a bit for me to wrap my head around, so I waited a bit to finish the review until I was fully recovered 😅 . It definitely made more sense when I was feeling better! And the namings were definitely confusing before your changes too

I have a few suggestions in-line below:

agilgur5 · 2023-10-25T21:21:42Z

ui/src/app/workflows/components/workflows-toolbar/workflows-toolbar.tsx

+                            onClick={() => {
+                                action.workflowsAction().catch();
+                            }}


Suggested change

onClick={() => {

action.workflowsAction().catch();

}}

onClick={action.workflowsAction}

this can be simplified and optimized a tiny bit. the previous .catch() is redundant since it doesn't do anything

agilgur5 · 2023-10-25T21:25:43Z

ui/src/app/workflows/components/workflows-toolbar/workflows-toolbar.tsx

-    }
+export function WorkflowsToolbar(props: WorkflowsToolbarProps) {
+    const {popup, notifications} = useContext(Context);
+    const numberSelected: number = props.selectedWorkflows.size;


Suggested change

const numberSelected: number = props.selectedWorkflows.size;

const numberSelected = props.selectedWorkflows.size;

I believe this should be inferred properly, no?

agilgur5 · 2023-10-27T22:15:45Z

ui/src/app/workflows/components/workflows-toolbar/workflows-toolbar.tsx

+                    // check for delete from archived workflows
+                    let deleteArchived = false;
+                    if (action.title === 'DELETE') {
+                        // check for delete workflows from archived workflows


Suggested change

// check for delete workflows from archived workflows

// check if there are archived workflows to delete

be a bit more specific and less redundant

agilgur5 · 2023-10-27T22:16:02Z

ui/src/app/workflows/components/workflows-toolbar/workflows-toolbar.tsx

-        if (!confirmed) {
-            return Promise.resolve(false);
-        }
+                    // check for delete from archived workflows


Suggested change

// check for delete from archived workflows

this is duplicative of the below comment

agilgur5 · 2023-10-27T22:17:24Z

ui/src/app/workflows/components/workflows-toolbar/workflows-toolbar.tsx

+                isDisabled: disabled[actionName],
+                action,
+                workflowsAction: async () => {
+                    // check for action


Suggested change

// check for action

this seems redundant to me since the word "confirm" is already used multiple times in the code and in the message

I think it seems redundant and unnecessary, too!

agilgur5 · 2023-10-27T22:23:58Z

ui/src/app/workflows/components/workflows-toolbar/workflows-toolbar.tsx

-                        content: `Performed '${action.title}' on selected workflows.`,
-                        type: NotificationType.Success
-                    });
-                    this.props.loadWorkflows();


so I actually don't think we should remove loadWorkflows at this point if we plan to replace it with an optimistic update per #12008. this specific call would be replaced with another call that takes the action as an argument

it would make the future change to an optimistic update more straightforward if we left this in

agilgur5 · 2023-10-27T22:35:51Z

ui/src/app/workflows/components/workflows-toolbar/workflows-toolbar.tsx

@@ -10,66 +11,71 @@ require('./workflows-toolbar.scss');

 interface WorkflowsToolbarProps {
    selectedWorkflows: Map<string, Workflow>;
-    loadWorkflows: () => void;
-    isDisabled: Actions.OperationDisabled;
+    actionsIsDisabled: Actions.OperationDisabled;


Suggested change

actionsIsDisabled: Actions.OperationDisabled;

disabledActions: Actions.OperationDisabled;

that probably makes a bit more sense since it's a mapping

agilgur5 · 2023-10-27T22:36:47Z

ui/src/app/workflows/components/workflows-toolbar/workflows-toolbar.tsx

+            return {
+                title: action.title,
+                iconClassName: action.iconClassName,
+                isDisabled: disabled[actionName],


Suggested change

isDisabled: disabled[actionName],

isDisabled: props.disabledActions[actionName],

we can just in-line this as it's only used here. no renames etc needed then

agilgur5 · 2023-10-27T22:49:38Z

ui/src/app/workflows/components/workflows-toolbar/workflows-toolbar.tsx

-    groupAction: () => Promise<any>;
+interface WorkflowsOperation extends WorkflowOperation {
+    isDisabled: boolean;
+    workflowsAction: () => Promise<any>;


Suggested change

workflowsAction: () => Promise<any>;

action: () => Promise<any>;

I think we can just overwrite action, since it's not used otherwise. the interface is already plural Workflows, so I don't think we need to repeat it again inside of it

Umm Then, Should we rename the name ofaction in below the code? Like this!

Before

<div className={`workflows-toolbar ${numberSelected === 0 ? 'hidden' : ''}`}> <div className='workflows-toolbar__count'> {numberSelected === 0 ? 'No' : numberSelected}  workflow{numberSelected === 1 ? '' : 's'} selected </div> <div className='workflows-toolbar__actions'> {operations.map(action => { return ( <button key={action.title} onClick={() => { action.workflowsAction(); }} className={`workflows-toolbar__actions--${action.title} workflows-toolbar__actions--action`} disabled={numberSelected === 0 || action.isDisabled}> <i className={action.iconClassName} />  {action.title} </button> ); })} </div> </div>

After

<div className={`workflows-toolbar ${numberSelected === 0 ? 'hidden' : ''}`}> <div className='workflows-toolbar__count'> {numberSelected === 0 ? 'No' : numberSelected}  workflow{numberSelected === 1 ? '' : 's'} selected </div> <div className='workflows-toolbar__actions'> {operations.map(operation => { return ( <button key={operation.title} onClick={() => { operation.action(); }} className={`workflows-toolbar__actions--${operation.title} workflows-toolbar__actions--action`} disabled={numberSelected === 0 || operation.isDisabled}> <i className={operation.iconClassName} />  {operation.title} </button> ); })} </div> </div>

Yup, that would be required

Note that with the onClick optimization in my above comment, those lines become onClick={operation.action}

Note that with the onClick optimization in my above comment, those lines become onClick={operation.action}

Oh I missed it, thank you!

agilgur5 · 2023-10-27T22:51:44Z

ui/src/app/workflows/components/workflows-toolbar/workflows-toolbar.tsx

-            </Consumer>
-        );
-    }
+    const groupAction = useMemo<WorkflowsOperation[]>(() => {


Suggested change

const groupAction = useMemo<WorkflowsOperation[]>(() => {

const groupActions = useMemo<WorkflowsOperation[]>(() => {

this should be plural since there are multiple actions in here

or maybe better, we could rename it to operations to match the type names

Suggested change

const groupAction = useMemo<WorkflowsOperation[]>(() => {

const operations = useMemo<WorkflowsOperation[]>(() => {

I think it's better to change the name to 'operations' to match the type name. Actually, I was confused that why the type was Operation but the name was groupAction.

Signed-off-by: juijeong8324 <juijeong8324@gmail.com>

juijeong8324 · 2023-11-01T15:26:56Z

The renames and re-ordering took a bit for me to wrap my head around, so I waited a bit to finish the review until I was fully recovered 😅 . It definitely made more sense when I was feeling better! And the namings were definitely confusing before your changes too

Thanks for detailed advice!!
The most difficult in refactoring is obviously naming value and parameter... 😂

agilgur5 · 2023-11-02T15:22:00Z

The most difficult in refactoring is obviously naming value and parameter... 😂

"There are only two hard things in Computer Science: cache invalidation and naming things." -- Phil Karlton

naming is indeed a hard thing, no joke 😉

agilgur5

LGTM 🚀 Thanks for iterating on this, getting the refactor done, and improving naming while you were at it!

juijeong8324 added 2 commits October 20, 2023 20:19

refactor: WorkflowsToolbar component from class to functional

5ebb36a

Signed-off-by: juijeong8324 <juijeong8324@gmail.com>

fix: modify annotation for lint

64de05a

Signed-off-by: juijeong8324 <juijeong8324@gmail.com>

agilgur5 self-requested a review October 20, 2023 15:48

agilgur5 added the area/ui label Oct 20, 2023

agilgur5 mentioned this pull request Oct 21, 2023

fix(ui): don't reload the page until _after_ deletion #11711

Merged

agilgur5 reviewed Oct 27, 2023

View reviewed changes

refactor: rename and re-ordering in WorkflowsToolbar

e4d0927

Signed-off-by: juijeong8324 <juijeong8324@gmail.com>

agilgur5 approved these changes Nov 2, 2023

View reviewed changes

agilgur5 requested a review from terrytangyuan November 2, 2023 15:24

terrytangyuan approved these changes Nov 2, 2023

View reviewed changes

terrytangyuan merged commit 4d062d1 into argoproj:master Nov 2, 2023
15 checks passed

agilgur5 mentioned this pull request Nov 7, 2023

UI: Refactor to use functional components #9810

Open

53 tasks

agilgur5 mentioned this pull request Feb 1, 2024

REQUEST: Promotion to Approver for @agilgur5 argoproj/argoproj#277

Closed

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(ui): WorkflowsToolbar component from class to functional #12046

refactor(ui): WorkflowsToolbar component from class to functional #12046

juijeong8324 commented Oct 20, 2023

agilgur5 left a comment •

edited

Loading

agilgur5 Oct 25, 2023

agilgur5 Oct 25, 2023

agilgur5 Oct 27, 2023 •

edited

Loading

agilgur5 Oct 27, 2023

agilgur5 Oct 27, 2023

juijeong8324 Oct 31, 2023

agilgur5 Oct 27, 2023

agilgur5 Oct 27, 2023

agilgur5 Oct 27, 2023

agilgur5 Oct 27, 2023

juijeong8324 Oct 31, 2023 •

edited

Loading

agilgur5 Oct 31, 2023 •

edited

Loading

juijeong8324 Nov 1, 2023

agilgur5 Oct 27, 2023

agilgur5 Oct 27, 2023

juijeong8324 Oct 31, 2023

juijeong8324 commented Nov 1, 2023

agilgur5 commented Nov 2, 2023

agilgur5 left a comment

	const numberSelected: number = props.selectedWorkflows.size;
	const numberSelected = props.selectedWorkflows.size;

	// check for delete workflows from archived workflows
	// check if there are archived workflows to delete

	actionsIsDisabled: Actions.OperationDisabled;
	disabledActions: Actions.OperationDisabled;

	isDisabled: disabled[actionName],
	isDisabled: props.disabledActions[actionName],

	workflowsAction: () => Promise<any>;
	action: () => Promise<any>;

	const groupAction = useMemo<WorkflowsOperation[]>(() => {
	const groupActions = useMemo<WorkflowsOperation[]>(() => {

	const groupAction = useMemo<WorkflowsOperation[]>(() => {
	const operations = useMemo<WorkflowsOperation[]>(() => {

refactor(ui): WorkflowsToolbar component from class to functional #12046

refactor(ui): WorkflowsToolbar component from class to functional #12046

Conversation

juijeong8324 commented Oct 20, 2023

Motivation

Modifications

Verification

agilgur5 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agilgur5 Oct 27, 2023 • 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

juijeong8324 Oct 31, 2023 • edited Loading

Choose a reason for hiding this comment

agilgur5 Oct 31, 2023 • 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

juijeong8324 commented Nov 1, 2023

agilgur5 commented Nov 2, 2023

agilgur5 left a comment

Choose a reason for hiding this comment

agilgur5 left a comment •

edited

Loading

agilgur5 Oct 27, 2023 •

edited

Loading

juijeong8324 Oct 31, 2023 •

edited

Loading

agilgur5 Oct 31, 2023 •

edited

Loading