-
Notifications
You must be signed in to change notification settings - Fork 1.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
Make PlainInput controllable #13795
Merged
Merged
Make PlainInput controllable #13795
Changes from all commits
Commits
Show all changes
17 commits
Select commit
Hold shift + click to select a range
13b6192
WIP
buoyad 4004de4
better typing for plaininput and type casting function
buoyad fa109e5
finish transform text and changes from testing
buoyad 7e6e920
hook up buildingPayment.amount to send for asset input, add story, an…
buoyad b0a3344
change api a bit
buoyad 93471da
add errors for illegal method use
buoyad f5ce402
forward ref from NewInput to PlainInput
buoyad 3f33463
add link to flow issue with forwardRef
buoyad bb6a720
remove castProps and use destructure in NewInput instead
buoyad 309eaa3
storyshots
buoyad 820d9ca
remove outdated comment
buoyad 368bc23
fix storyshots from jest cache
buoyad d5862e4
Merge branch 'master' into danny/DESKTOP-7572-new-controllable-inputs
buoyad 537429f
add getSelection and example of changing text and selection at same time
buoyad 05d5357
review feedback
buoyad e80be0e
native getProps cleanup
buoyad bbc8dc8
better check for controlled
buoyad File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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 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 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 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 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
Oops, something went wrong.
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.
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.
Hmm...so now I'm confused by this API. We almost always want to change both the text and the selection at the same time...AIUI the existing use cases are inserting text (like emoji, mentions, etc.) at the cursor. So that's why I suggested having a
selection
prop along with the value. Did that not work?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 ran into issues early on with the parent actively managing the selection and value at the same time. AFAICT on desktop there's no way to listen to events that change the cursor position (without selecting anything). Without the parent knowing the selection beforehand, I think the semantics of the prop would be weird, and even then I'm not sure if there's a way to insert text at the cursor the way we want. I'm beginning to think having a
transformText
that works with controlled inputs would be a good idea, though I'm not sure how to handle that along with updatingprops.value
...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.
Ah, yes, I remember running into that when I tried. @chrisnojima did mention there was some way to listen to all DOM events and then filter out the selection ones on a given component, but I didn't know enough to pursue that.
...and then, yeah, you can try implementing
transformText
on a controlled component, but then things get murky w.r.t. the source of truth...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.
@akalin-keybase OK, rather than going down the murky source of truth route, I added a
getSelection
method. I added an example of a controlled input setting both at the same time (getSelection
-> changeprops.value
->setSelection
). Let me know what you think