refactor(ui): convert a few components to use hooks #11800
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- `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 <agilgur5@gmail.com>
- `this.state` -> `useState` - `this.props` -> `props` - `componentDidMount` -> `useEffect` - also convert to async/await syntax Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
- `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 <agilgur5@gmail.com>
- consistent with other parts of the codebase and previous refactors, such as 25e1939 Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
- 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 <agilgur5@gmail.com>
agilgur5
force-pushed
the
refactor-ui-hooks
branch
from
bf3e2a5
to
7c7da9b
Compare
53 tasks
toyamagu-2021
approved these changes
Sep 13, 2023
terrytangyuan
approved these changes
Sep 21, 2023
This was referenced Sep 28, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Partial fix for #9810
Partial fix for #11740
Motivation
Use newer, modern React hooks syntax instead of legacy React component lifecycle hooks
Use newer, modern async/await syntax instead of Promise chains
Combined a few components together as previously requested. These are all components that make use of effects and other hooks (e.g. refs), but are not significantly complicated (unlike #11794, which is one complex component -- an entire page. and #11791 is all components without effects).
Modifications
Convert 3 components to use hooks:
ResourceEditor,WorkflowDrawer, andTagsInputthis.props->propsthis.state->useStateref->useRef.currentcomponentDidMount->useEffectmethods -> inner functions
also shorten some logic by using modern optional chaining syntax
See commit messages for more details
Verification
TagsInput(as the refs gave some errors for a bit due to an accidental incorrect renaming) and a bit forWorkflowDrawer. Screenshots of them working post-refactor:Notes for Reviewers
Ignoring whitespace changes makes it a bit easier to read. Though there are still a good amount of other refactorings