From e0e49b91455db110f0f33e9a4802ad070ea95500 Mon Sep 17 00:00:00 2001 From: Anton Gilgur Date: Sun, 10 Sep 2023 18:07:57 -0400 Subject: [PATCH 1/6] refactor(ui): convert `ResourceEditor` to use hooks - `this.props` -> `props` - `this.state` -> `useState` - simplify some callbacks now too - methods -> inner functions - also convert to async/await syntax as well - hoist child render functions into the main component - could possibly optimize this a bit with `useCallback` - and there's bit of a logic bug in `submit` as it sets `value.metadata.namespace` directly - but this was present in the previous version already. and it probably gets quickly overwritten / re-rendered by `props.onSubmit`. - would be good to fix just in case though, but this component is rarely used too Signed-off-by: Anton Gilgur --- .../resource-editor/resource-editor.tsx | 103 ++++++++---------- 1 file changed, 44 insertions(+), 59 deletions(-) diff --git a/ui/src/app/shared/components/resource-editor/resource-editor.tsx b/ui/src/app/shared/components/resource-editor/resource-editor.tsx index 264d3eb935bb..4e4d077fe0ab 100644 --- a/ui/src/app/shared/components/resource-editor/resource-editor.tsx +++ b/ui/src/app/shared/components/resource-editor/resource-editor.tsx @@ -1,5 +1,6 @@ import * as kubernetes from 'argo-ui/src/models/kubernetes'; import * as React from 'react'; +import {useState} from 'react'; import {Button} from '../button'; import {ErrorNotice} from '../error-notice'; import {ObjectEditor} from '../object-editor/object-editor'; @@ -15,69 +16,53 @@ interface Props { onSubmit?: (value: T) => Promise; } -interface State { - editing: boolean; - value: T; - error?: Error; -} - -export class ResourceEditor extends React.Component, State> { - constructor(props: Readonly>) { - super(props); - this.state = {editing: this.props.editing, value: this.props.value}; - } +export function ResourceEditor(props: Props) { + const [editing, setEditing] = useState(props.editing); + const [value, setValue] = useState(props.value); + const [error, setError] = useState(); - public render() { - return ( - <> - {this.props.title &&

{this.props.title}

} - {this.state.error && } - this.setState({value})} - /> - - ); + async function submit() { + try { + if (value.metadata && !value.metadata.namespace && props.namespace) { + value.metadata.namespace = props.namespace; + } + await props.onSubmit(value); + setError(null); + } catch (newError) { + setError(newError); + } } - private renderButtons() { - return ( - <> - {this.state.editing ? ( + return ( + <> + {props.title &&

{props.title}

} + + - {this.props.upload && onUpload={value => this.setState({value})} onError={error => this.setState({error})} />} - {this.props.onSubmit && ( - + {editing ? ( + <> + {props.upload && onUpload={setValue} onError={setError} />} + {props.onSubmit && ( + + )} + + ) : ( + props.onSubmit && ( + + ) )} - ) : ( - this.props.onSubmit && ( - - ) - )} - - ); - } - - private submit() { - try { - const value = this.state.value; - if (value.metadata && !value.metadata.namespace && this.props.namespace) { - value.metadata.namespace = this.props.namespace; - } - this.props - .onSubmit(value) - .then(() => this.setState({error: null})) - .catch(error => this.setState({error})); - } catch (error) { - this.setState({error}); - } - } + } + onChange={setValue} + /> + + ); } From e67635c2b531402cea4b0591d4dbb8a0aab81585 Mon Sep 17 00:00:00 2001 From: Anton Gilgur Date: Sun, 10 Sep 2023 18:40:58 -0400 Subject: [PATCH 2/6] refactor(ui): convert `WorkflowDrawer` to use hooks - `this.state` -> `useState` - `this.props` -> `props` - `componentDidMount` -> `useEffect` - also convert to async/await syntax Signed-off-by: Anton Gilgur --- .../workflow-drawer/workflow-drawer.tsx | 175 +++++++++--------- 1 file changed, 83 insertions(+), 92 deletions(-) diff --git a/ui/src/app/workflows/components/workflow-drawer/workflow-drawer.tsx b/ui/src/app/workflows/components/workflow-drawer/workflow-drawer.tsx index 1c03a964ca3a..8e6c4f589858 100644 --- a/ui/src/app/workflows/components/workflow-drawer/workflow-drawer.tsx +++ b/ui/src/app/workflows/components/workflow-drawer/workflow-drawer.tsx @@ -1,4 +1,5 @@ import * as React from 'react'; +import {useEffect, useState} from 'react'; import {Workflow} from '../../../../models'; import {InlineTable} from '../../../shared/components/inline-table/inline-table'; @@ -18,109 +19,99 @@ interface WorkflowDrawerProps { onChange: (key: string) => void; } -interface WorkflowDrawerState { - workflow?: Workflow; -} +export function WorkflowDrawer(props: WorkflowDrawerProps) { + const [wf, setWorkflow] = useState(); -export class WorkflowDrawer extends React.Component { - constructor(props: Readonly) { - super(props); - this.state = {}; - } + useEffect(() => { + (async () => { + const newWf = await services.workflows.get(props.namespace, props.name) + setWorkflow(newWf); + })(); + }, [props.namespace, props.name]); - public componentDidMount() { - services.workflows.get(this.props.namespace, this.props.name).then(workflow => { - this.setState({workflow}); - }); + if (!wf) { + return ; } - public render() { - const wf = this.state.workflow; - - if (!wf) { - return ; - } - - return ( -
- {!wf.status || !wf.status.message ? null : ( -
-
MESSAGE
-
{wf.status.message}
-
- )} - {!wf.status || !wf.status.conditions ? null : ( -
-
CONDITIONS
-
- -
+ return ( +
+ {!wf.status || !wf.status.message ? null : ( +
+
MESSAGE
+
{wf.status.message}
+
+ )} + {!wf.status || !wf.status.conditions ? null : ( +
+
CONDITIONS
+
+
- )} - {!wf.status || !wf.status.resourcesDuration ? null : ( -
- + )} + {!wf.status || !wf.status.resourcesDuration ? null : ( +
+ + ), + right: ( +
+
+ {formatDuration(wf.status.resourcesDuration.cpu, 1)} + (*1 CPU)
- ), - right: (
-
- {formatDuration(wf.status.resourcesDuration.cpu, 1)} - (*1 CPU) -
-
- {formatDuration(wf.status.resourcesDuration.memory, 1)} - (*100Mi Memory) -
+ {formatDuration(wf.status.resourcesDuration.memory, 1)} + (*100Mi Memory)
- ) - } - ]} - /> -
-
- )} -
-
FROM
-
- -
-
-
-
LABELS
-
- { - this.props.onChange(key); - }} +
+ ) + } + ]} />
-
-
Creator
-
- { - this.props.onChange(key); - }} - /> -
+ )} +
+
FROM
+
+
- ); - } +
+
LABELS
+
+ { + props.onChange(key); + }} + /> +
+
+
+
Creator
+
+ { + props.onChange(key); + }} + /> +
+
+
+ ); } From 9ba2fcfb0fedcd82e0b7b3e6cff0b658ae7a35ca Mon Sep 17 00:00:00 2001 From: Anton Gilgur Date: Sun, 10 Sep 2023 18:42:52 -0400 Subject: [PATCH 3/6] refactor(ui): convert `TagsInput` to use hooks - `this.props` -> `props` - `this.state` -> `useState` - methods -> inner functions - hoist `onTagsChange` to above render - also convert to async/await syntax - `ref` -> `useRef` - rename variables to clarify that these are refs which are accessed via `.current` - simplify `inputRef` callback as we can now pass it directly - `autoCompleteRef` could not be passed directly as the `argo-ui` component does not quite support that - likely need to conver that to use `forwardRef`. I'm imaginging that `argo-ui` might all be `class` components? (as it hasn't been maintained in some time) - rename `renderInput` render props to `inputProps` so as to not conflict with top-level `props` naming - also shorten some logic by using optional chaining syntax Signed-off-by: Anton Gilgur --- .../components/tags-input/tags-input.tsx | 199 ++++++++---------- 1 file changed, 92 insertions(+), 107 deletions(-) diff --git a/ui/src/app/shared/components/tags-input/tags-input.tsx b/ui/src/app/shared/components/tags-input/tags-input.tsx index bb988511c9b5..d4bd35175d3a 100644 --- a/ui/src/app/shared/components/tags-input/tags-input.tsx +++ b/ui/src/app/shared/components/tags-input/tags-input.tsx @@ -1,5 +1,6 @@ import classNames from 'classnames'; import * as React from 'react'; +import {useRef, useState} from 'react'; import {Autocomplete, AutocompleteApi, AutocompleteOption} from 'argo-ui'; @@ -13,116 +14,100 @@ interface TagsInputProps { require('./tags-input.scss'); -export class TagsInput extends React.Component { - private inputElement: HTMLInputElement; - private autocompleteApi: AutocompleteApi; +export function TagsInput(props: TagsInputProps) { + const inputRef = useRef(null); + const autoCompleteRef = useRef(null); - constructor(props: TagsInputProps) { - super(props); - this.state = {tags: props.tags || [], input: '', focused: false, subTags: [], subTagsActive: false}; + const [tags, setTags] = useState(props.tags || []); + const [input, setInput] = useState(''); + const [focused, setFocused] = useState(false); + const [subTags, setSubTags] = useState([]); + const [subTagsActive, setSubTagsActive] = useState(false); + + function onTagsChange(tags: string[]) { + if (props.onChange) { + props.onChange(tags); + setTimeout(() => autoCompleteRef.current?.refresh()); + } } - public render() { - return ( -
this.inputElement && this.inputElement.focus()}> - {this.props.tags ? ( - this.props.tags.map((tag, i) => ( - - {tag}{' '} - { - const newTags = [...this.state.tags.slice(0, i), ...this.state.tags.slice(i + 1)]; - this.setState({tags: newTags}); - this.onTagsChange(newTags); - e.stopPropagation(); - }} - /> - - )) - ) : ( - - )} - (this.autocompleteApi = api)} - value={this.state.input} - items={this.state.subTagsActive ? this.state.subTags : this.props.autocomplete} - onChange={e => this.setState({input: e.target.value})} - onSelect={value => { - if (this.props.sublistQuery != null && !this.state.subTagsActive) { - this.setState({subTagsActive: true}); - this.props.sublistQuery(value).then(list => { - this.setState({ - subTags: list || [] - }); - }); - } else { - if (this.state.tags.indexOf(value) === -1) { - const newTags = this.state.tags.concat(value); - this.setState({input: '', tags: newTags, subTags: []}); - this.onTagsChange(newTags); - } - this.setState({subTagsActive: false}); - } - }} - renderInput={props => ( - { - this.inputElement = inputEl; - if (props.ref) { - (props.ref as any)(inputEl); - } - }} - onFocus={e => { - if (props.onFocus) { - props.onFocus(e); - } - this.setState({focused: true}); - }} - onBlur={e => { - if (props.onBlur) { - props.onBlur(e); - } - this.setState({focused: false}); - }} - onKeyDown={e => { - if (e.keyCode === 8 && this.state.tags.length > 0 && this.state.input === '') { - const newTags = this.state.tags.slice(0, this.state.tags.length - 1); - this.setState({tags: newTags}); - this.onTagsChange(newTags); - } - if (props.onKeyDown) { - props.onKeyDown(e); - } - }} - onKeyUp={e => { - if (e.keyCode === 13 && this.state.input && this.state.tags.indexOf(this.state.input) === -1) { - const newTags = this.state.tags.concat(this.state.input); - this.setState({input: '', tags: newTags}); - this.onTagsChange(newTags); - } - if (props.onKeyUp) { - props.onKeyUp(e); - } + return ( +
inputRef.current?.focus()}> + {props.tags ? ( + props.tags.map((tag, i) => ( + + {tag}{' '} + { + const newTags = [...tags.slice(0, i), ...tags.slice(i + 1)]; + setTags(newTags); + onTagsChange(newTags); + e.stopPropagation(); }} /> - )} - /> -
- ); - } - - private onTagsChange(tags: string[]) { - if (this.props.onChange) { - this.props.onChange(tags); - if (this.autocompleteApi) { - setTimeout(() => this.autocompleteApi.refresh()); - } - } - } + + )) + ) : ( + + )} + (autoCompleteRef.current = ref)} + value={input} + items={subTagsActive ? subTags : props.autocomplete} + onChange={e => setInput(e.target.value)} + onSelect={async value => { + if (props.sublistQuery != null && !subTagsActive) { + setSubTagsActive(true); + const newSubTags = await props.sublistQuery(value); + setSubTags(newSubTags || []); + } else { + if (tags.indexOf(value) === -1) { + const newTags = tags.concat(value); + setTags(newTags); + setInput(''); + setSubTags([]); + onTagsChange(newTags); + } + setSubTagsActive(false); + } + }} + renderInput={inputProps => ( + { + inputProps.onFocus?.(e); + setFocused(true); + }} + onBlur={e => { + inputProps.onBlur?.(e); + setFocused(false); + }} + onKeyDown={e => { + if (e.keyCode === 8 && tags.length > 0 && input === '') { + const newTags = tags.slice(0, tags.length - 1); + setTags(newTags); + onTagsChange(newTags); + } + inputProps.onKeyDown?.(e); + }} + onKeyUp={e => { + if (e.keyCode === 13 && input && tags.indexOf(input) === -1) { + const newTags = tags.concat(input); + setTags(newTags); + setInput(''); + onTagsChange(newTags); + } + inputProps.onKeyUp?.(e); + }} + /> + )} + /> +
+ ); } From 3920693c810df64d08d1c1aaf786c2c5c9c7bab2 Mon Sep 17 00:00:00 2001 From: Anton Gilgur Date: Sun, 10 Sep 2023 18:55:54 -0400 Subject: [PATCH 4/6] refactor(ui): convert `TagsInput` callback to `useEffect` - consistent with other parts of the codebase and previous refactors, such as 25e1939e25551cd15d89bd47e4232c8073b40a9c Signed-off-by: Anton Gilgur --- ui/src/app/shared/components/tags-input/tags-input.tsx | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/ui/src/app/shared/components/tags-input/tags-input.tsx b/ui/src/app/shared/components/tags-input/tags-input.tsx index d4bd35175d3a..ad3ca83897a2 100644 --- a/ui/src/app/shared/components/tags-input/tags-input.tsx +++ b/ui/src/app/shared/components/tags-input/tags-input.tsx @@ -1,6 +1,6 @@ import classNames from 'classnames'; import * as React from 'react'; -import {useRef, useState} from 'react'; +import {useEffect, useRef, useState} from 'react'; import {Autocomplete, AutocompleteApi, AutocompleteOption} from 'argo-ui'; @@ -24,12 +24,12 @@ export function TagsInput(props: TagsInputProps) { const [subTags, setSubTags] = useState([]); const [subTagsActive, setSubTagsActive] = useState(false); - function onTagsChange(tags: string[]) { + useEffect(() => { if (props.onChange) { props.onChange(tags); setTimeout(() => autoCompleteRef.current?.refresh()); } - } + }, [props.onChange, tags, autoCompleteRef]); return (
{ const newTags = [...tags.slice(0, i), ...tags.slice(i + 1)]; setTags(newTags); - onTagsChange(newTags); e.stopPropagation(); }} /> @@ -70,7 +69,6 @@ export function TagsInput(props: TagsInputProps) { setTags(newTags); setInput(''); setSubTags([]); - onTagsChange(newTags); } setSubTagsActive(false); } @@ -92,7 +90,6 @@ export function TagsInput(props: TagsInputProps) { if (e.keyCode === 8 && tags.length > 0 && input === '') { const newTags = tags.slice(0, tags.length - 1); setTags(newTags); - onTagsChange(newTags); } inputProps.onKeyDown?.(e); }} @@ -101,7 +98,6 @@ export function TagsInput(props: TagsInputProps) { const newTags = tags.concat(input); setTags(newTags); setInput(''); - onTagsChange(newTags); } inputProps.onKeyUp?.(e); }} From 7e2d6afdd485fbb8448f7c8afceba799229af0c4 Mon Sep 17 00:00:00 2001 From: Anton Gilgur Date: Sun, 10 Sep 2023 19:11:56 -0400 Subject: [PATCH 5/6] fix lint Signed-off-by: Anton Gilgur --- ui/src/app/shared/components/tags-input/tags-input.tsx | 4 +--- .../workflows/components/workflow-drawer/workflow-drawer.tsx | 2 +- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/ui/src/app/shared/components/tags-input/tags-input.tsx b/ui/src/app/shared/components/tags-input/tags-input.tsx index ad3ca83897a2..9160a789d8ea 100644 --- a/ui/src/app/shared/components/tags-input/tags-input.tsx +++ b/ui/src/app/shared/components/tags-input/tags-input.tsx @@ -32,9 +32,7 @@ export function TagsInput(props: TagsInputProps) { }, [props.onChange, tags, autoCompleteRef]); return ( -
inputRef.current?.focus()}> +
inputRef.current?.focus()}> {props.tags ? ( props.tags.map((tag, i) => ( diff --git a/ui/src/app/workflows/components/workflow-drawer/workflow-drawer.tsx b/ui/src/app/workflows/components/workflow-drawer/workflow-drawer.tsx index 8e6c4f589858..9415e60d6c1f 100644 --- a/ui/src/app/workflows/components/workflow-drawer/workflow-drawer.tsx +++ b/ui/src/app/workflows/components/workflow-drawer/workflow-drawer.tsx @@ -24,7 +24,7 @@ export function WorkflowDrawer(props: WorkflowDrawerProps) { useEffect(() => { (async () => { - const newWf = await services.workflows.get(props.namespace, props.name) + const newWf = await services.workflows.get(props.namespace, props.name); setWorkflow(newWf); })(); }, [props.namespace, props.name]); From 7c7da9b9bd29c47879a142a4dffb8cc70990a54b Mon Sep 17 00:00:00 2001 From: Anton Gilgur Date: Mon, 11 Sep 2023 17:37:14 -0400 Subject: [PATCH 6/6] fix(ui): `TagsInput` ref needs to call `inputProps.ref` as well - without this, the input would throw an error when adding a label - this matches the old class-based code as well - I think I misread the old code while refactoring and missed that it was `inputProps.ref` and _not_ `inputRef` - in old code, it was `props.ref` and `this.inputEl`, so the refactor changed up names that crossed each other a bit Signed-off-by: Anton Gilgur --- ui/src/app/shared/components/tags-input/tags-input.tsx | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/ui/src/app/shared/components/tags-input/tags-input.tsx b/ui/src/app/shared/components/tags-input/tags-input.tsx index 9160a789d8ea..6e28b9b5f321 100644 --- a/ui/src/app/shared/components/tags-input/tags-input.tsx +++ b/ui/src/app/shared/components/tags-input/tags-input.tsx @@ -29,7 +29,7 @@ export function TagsInput(props: TagsInputProps) { props.onChange(tags); setTimeout(() => autoCompleteRef.current?.refresh()); } - }, [props.onChange, tags, autoCompleteRef]); + }, [tags]); return (
inputRef.current?.focus()}> @@ -75,7 +75,12 @@ export function TagsInput(props: TagsInputProps) { { + inputRef.current = ref; + if (typeof inputProps.ref === 'function') { + inputProps.ref(ref); + } + }} onFocus={e => { inputProps.onFocus?.(e); setFocused(true);