Tidy and minor refactor of Link UI code #37833
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
getdave
added
[Type] Code Quality
|
Size Change: -6 B (0%) Total Size: 1.29 MB
ℹ️ View Unchanged
|
youknowriad
reviewed
Jan 11, 2022
| */ | ||
| import { useState, useEffect } from '@wordpress/element'; | ||
|
|
||
| export default function useInternalInputValue( value ) { |
There was a problem hiding this comment.
Would using key to force remount the component be simpler in this context?
There was a problem hiding this comment.
for a use case like a different link in a RichText component, it potentially makes sense to force remount but it's a fairly common practice in reusable components to have "internal/external state and changes in external forcing updates to internal state"
There was a problem hiding this comment.
t's a fairly common practice in reusable components to have "internal/external state and changes in external forcing updates to internal state"
I'm not sure, it's essentially the same as a derived state, which isn't really common according to React core team. And seems like we can just call setState during the rendering phase if we really need to do this (which I'm not sure if is still the recommended approach in concurrent rendering 🤔 ).
There was a problem hiding this comment.
Yeh I always felt a little uncomfortable regarding this pattern here.
The key UX here is that the value shouldn't be committed until the user takes an explicit action (e.g. hitting Submit or ENTER). Until that point the data the user has typed should be considered local to the component because if the user clicks away we should be able to discard the change.
That is why the internal state exists.
There was a problem hiding this comment.
Would using key to force remount the component be simpler in this context?
When we use LinkControl we now often pass a key prop to force re-render the component when a "new" link is being handled (like when you move between links in a paragraph block).
gutenberg/packages/format-library/src/link/inline.js
Lines 236 to 237 in fab909c
diegohaz
reviewed
Jan 11, 2022
packages/block-editor/src/components/link-control/use-internal-input-value.js
Outdated
Show resolved
Hide resolved
getdave
force-pushed
the
try/minor-link-ui-code-tidy
branch
from
a6a1c8a to
f02c2ba
Compare
Co-authored-by: Haz <hazdiego@gmail.com>
getdave
force-pushed
the
try/minor-link-ui-code-tidy
branch
from
f0402e1 to
0f7b120
Compare
|
It would be great if we could get this refactor merged. I don't see any actionable feedback remaining here. |
scruffian
approved these changes
Nov 9, 2022
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.
Description
The Link UI code had become a bit jumbled and inordly. This PR
How has this been tested?
Check Link UI works as before.
Check all tests pass.
Screenshots
Types of changes
Checklist:
*.native.jsfiles for terms that need renaming or removal).