-
Notifications
You must be signed in to change notification settings - Fork 2.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[core] add AsyncControllableTextArea based on AsyncControllableInput & add async control option for TextArea component #6312
Conversation
add correct types & copy tests from default inputBuild artifact links for this commit: documentation | landing | table | demoThis is an automated comment from the deploy-preview CircleCI job. |
fix lint, object orderBuild artifact links for this commit: documentation | landing | table | demoThis is an automated comment from the deploy-preview CircleCI job. |
// NOTE: these are copied from the React.HTMLAttributes interface definition. | ||
onChange?: React.ChangeEventHandler<AsyncControllableElement<T>> | undefined; | ||
onCompositionStart?: React.CompositionEventHandler<AsyncControllableElement<T>> | undefined; | ||
onCompositionEnd?: React.CompositionEventHandler<AsyncControllableElement<T>> | undefined; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure if this is the best way to do it, but I would argue this is fine since this component is not exported directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not want to further complicate the types by excluding and extending the native interfaces above.
refactor testsBuild artifact links for this commit: documentation | landing | table | demoThis is an automated comment from the deploy-preview CircleCI job. |
Hi @gluxon, any chance you could take a look a this? Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies for the late response — It took me a bit of time to come around to it, but I like how this takes advantage of generic types to ensure type safety of the input props.
A discussion of a long-term approach may make sense. Zooming out a bit, more recent React code tends to lean on hooks for component logic sharing. I think the need to add additional functionality to this existing AsyncControllableInput
class component comes from the fact that it was written when component composition was a more common logic reuse pattern. In that prior world, this PR makes sense.
If we were to imagine an alternative hooks-based approach, I think adding support for new HTML textual input elements becomes significantly easier. This example isn't complete and likely fails to compile, but here's the rough direction I was thinking:
function AsyncControllableInput(props: P) {
const { value, onChange, ...rest } = props;
const { textInputProps } = useAsyncControlledValue({ value, onChange });
return <input {...rest} {...textInputProps} />
}
function AsyncControllableTextarea(props: P) {
const { value, onChange, ...rest } = props;
const { textInputProps } = useAsyncControlledValue({ value, onChange });
return <textarea {...rest} {...textInputProps} />
}
function useAsyncControllableValue(opts) {
const [isComposing, setIsComposing] = React.useState(false);
const [nextValue, setNextValue ] = React.useState(/* ... */);
const [hasPendingUpdate, setHasPendingUpdate ] = React.useState(/* ... */);
function onCompositionStart() {
setIsComposing(true)
}
function onCompositionEnd() {
setIsComposing(false)
}
const value = isComposing || hasPendingUpdate ? nextValue : value
/* ... */
return {
value,
onChange,
onCompositionEnd,
onCompositionStart
};
}
Since this PR adds a new functionality, I do wonder if it makes sense to create a new AsyncControllableTextarea
that's based upon the hook approach. We would then battle-test that purely new functionality before switching AsyncControllableInput
to be implemented from the same hook in a future Blueprint release.
I'm also okay extending the existing AsyncControllableInput
to be generic since it's private anyway. I do think the hook approach is the preferable long-term approach though. What do you think?
Basically the only thing I was afraid of and tried to avoid is any external incompatibilities with the API and component props types. But since
AsyncControllableInput
is not exported externally and only used as a part of other components I think we can get away with the introduced complex types.
I appreciate that! I scanned through the PR looking for changes to external APIs and don't see any other than the new asyncControl
prop that was intended.
describe("<AsyncControllableInput>", () => { | ||
runAsyncControllableInputTests({ tagName: "input", inputType: "text" }); | ||
runAsyncControllableInputTests({ tagName: "textarea" }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the test deduplication here. Thanks for doing that.
What are your thoughts on describe.each
? That would be another way to write this in more idiomatic Jest. https://jestjs.io/docs/api#describeeachtablename-fn-timeout
@@ -170,13 +190,22 @@ export class TextArea extends AbstractPureComponent<TextAreaProps, TextAreaState | |||
}; | |||
} | |||
|
|||
return ( | |||
return asyncControl ? ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this seem to work when testing live? The code changes look correct. Just wondering if <TextArea asyncControl />
works as expected.
I really do like the hooks approach! That's what I was aiming for at first, but then gave up unfortunately. In any case, I will revisit this again with hooks. Thanks for reviewing! |
Please reach out if you want a second pair of eyes while looking at this! 🙂 |
Thanks! I updated the PR with hooks alternative based on the |
add more commentsBuild artifact links for this commit: documentation | landing | table | demoThis is an automated comment from the deploy-preview CircleCI job. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
packages/core/src/components/forms/asyncControllableTextArea.tsx
Outdated
Show resolved
Hide resolved
packages/core/src/components/forms/asyncControllableTextArea.tsx
Outdated
Show resolved
Hide resolved
packages/core/src/components/forms/useAsyncControllableValue.ts
Outdated
Show resolved
Hide resolved
if (propValue === nextValue) { | ||
// parent has processed and accepted our update | ||
if (hasPendingUpdate) { | ||
setValue(propValue); | ||
setHasPendingUpdate(false); | ||
} else if (value !== nextValue) { | ||
setValue(nextValue); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There might be opportunity to simplify this block. I know it was copied from the original version, but since we're rewriting it anyway, it might be cool to revisit.
Staring at this part, I think I can convince myself that these blocks are equivalent:
if (propValue === nextValue) {
// parent has processed and accepted our update
if (hasPendingUpdate) {
setValue(propValue);
setHasPendingUpdate(false);
} else if (value !== nextValue) {
setValue(nextValue);
}
} else {
if (propValue === nextValue) {
// parent has processed and accepted our update
setValue(propValue);
setHasPendingUpdate(false);
} else {
The theory being that
propValue
andnextValue
are the same, so either can be used tosetValue
.- React should no-op the
setHasPendingUpdate(false)
call if it's alreadyfalse
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Taking this a bit further, I think the nested logic can be much flatter.
if (!isStateDerivationPaused) {
const userTriggeredUpdate = nextValue !== value;
if (userTriggeredUpdate && propValue === nextValue) {
// parent has processed and accepted our update
setValue(propValue);
setHasPendingUpdate(false);
} else if (userTriggeredUpdate && propValue === value) {
// we have sent the update to our parent, but it has not been processed yet. just wait.
// DO NOT set nextValue here, since that will temporarily render a potentially stale controlled value,
// causing the cursor to jump once the new value is accepted
if (!hasPendingUpdate) {
setHasPendingUpdate(true);
}
} else if (userTriggeredUpdate || value !== propValue || hasPendingUpdate) {
// accept controlled update, could be confirming or denying user action
setValue(propValue);
setNextValue(propValue);
setHasPendingUpdate(false);
}
}
But I could definitely use help making sure I refactored that correctly. The general strategy was to propagate the userTriggeredUpdate
condition down and then remove layers of nesting.
If you would rather not think about further refactors and prefer to keep the existing structure as-is to match the current getDerivedStateFromProps
approach, I think that's okay too!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I refactored this bit to be flatter and (arguably) more readable than the initial getDerivedStateFromProps
version in the latest commit. Thanks for the ideas above!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
packages/core/src/components/forms/useAsyncControllableValue.ts
Outdated
Show resolved
Hide resolved
Thanks for the review @gluxon! Updated based on the latest comments / suggestions 👍 |
flatten & simplify state derival + commentsBuild artifact links for this commit: documentation | landing | table | demoThis is an automated comment from the deploy-preview CircleCI job. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the updates! I think everything looks good. Expecting to approve and merge once the discussion about tests https://github.com/palantir/blueprint/pull/6312/files/bdc07b6e4a19ef154d84cbc233943985c6f91cbd#r1300068035 wraps up.
Updated the tests & added a clarifying note at the top of the test file |
dedup tests & add noteBuild artifact links for this commit: documentation | landing | table | demoThis is an automated comment from the deploy-preview CircleCI job. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the improvements and new feature @nurseiit! 🎉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
||
import * as React from "react"; | ||
|
||
interface IUseAsyncControllableValueProps<E extends HTMLInputElement | HTMLTextAreaElement> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: same comment about avoiding the I
prefix
Related to #4678.
Checklist
Changes proposed in this pull request:
a. (
Extends the existing) -- initial approach. Ended up with aAsyncControllableInput
with an option to render atextarea
element. (the type of Props is quite complicated now, and I am open to suggestions to further simplify it)useAsyncControllableValue
hook to use inAsyncControllableTextArea
component.b. Adds
asyncControl
option toTextArea
component thatreusesusesAsyncControllableInput
AsyncControllableTextArea
when set totrue
.Reviewers should focus on:
Basically the only thing I was afraid of and tried to avoid is any external incompatibilities with the API and component props types. But since
AsyncControllableInput
is not exported externally and only used as a part of other components I think we can get away with the introduced complex types.Also I generalised the tests for
AsyncControllableInput
instead of copying them, one can argue this was unnecessary, but I am also open for any suggestions on this.Thanks!