Skip to content
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

Clear input when we are adjusting frame #77

Merged
merged 1 commit into from
Sep 11, 2015

Conversation

walsh2000
Copy link

This resolves issue #76
When we are laying out subviews, we were already passing NO for adjustFrame.
When we are reloading data, we were already passing YES for adjustFrame.

It turns out that when we are not adjustingFrame, we do not want to clear the user-entered text, so reuse this boolean to keep from clearing the user text.

This resolves issue venmo#76
When we are laying out subviews, we were already passing NO for adjustFrame.
When we are reloading data, we were already passing YES for adjustFrame.

It turns out that when we are not adjustingFrame, we do not want to clear the user-entered text, so reuse this boolean to keep from clearing the user text.
@walsh2000
Copy link
Author

Has anyone had a moment to consider this PR?

@walsh2000
Copy link
Author

@ayanonagon

You're a significant contributor to this repo, and you seem to still be contributing to other venmo repos.
Do you know if this repo is dead? Might this PR get reviewed & possibly included, or do you think I should live off of my own fork?

Thank you for any advice
Ray

@ayanonagon
Copy link
Contributor

Hi @walsh2000, no this repo is not dead. It’s been a bit difficult to keep up with all the issues and PR’s but we’re still using this in the Venmo codebase so it’s definitely not dead. Sorry for the lateness!

Regarding this PR, it appears to a breaking change in that would affect other developers using this since the input is clearing on frame changes. Can you walk me through what the use case is? Thanks! :octocat:

@walsh2000
Copy link
Author

@ayanonagon

Great news! Thanks for the reply.

Before this PR, the VENTokenField will clear the text field when the VC rotates (portrait->landscape for example)

Steps:

  1. Type a few characters, but don't turn the text into a token yet
  2. Rotate device
  3. Typed text is lost

This PR does not clear the inputTextField.text when we're simply laying out subviews.
Notice that there are 3 callers of -[layoutTokensAndInputWithFrameAdjustment:]
I did not modify any of these call sites. -[layoutSubviews] passes "NO" to layoutTokensAndInputWithFrameAdjustment, and that was the boolean I needed to prevent clearing the inputTextField.text.

It seems unlikely that existing clients would be relying on the text field being cleared in response to layoutSubviews. If you believe there are clients, I can add a property to clear/not-clear the textField on layoutSubviews. (Making the default be "do not clear"-- old behavior)
Would that make you more comfortable?

Thank you again for your time
Ray

@ayanonagon
Copy link
Contributor

Gotcha, thanks for clarifying. Will take a better look when I get to my
computer and get it merged. Thanks again!
On Tue, Sep 8, 2015 at 13:12, Raymond Walsh [email protected]
wrote:

@ayanonagon https://github.com/ayanonagon

Great news! Thanks for the reply.

Before this PR, the VENTokenField will clear the text field when the VC
rotates (portrait->landscape for example)

Steps:

  1. Type a few characters, but don't turn the text into a token yet
  2. Rotate device
  3. Typed text is lost

This PR does not clears the inputTextField.text when we're simply laying
out subviews.
Notice that there are 3 callers of
-[layoutTokensAndInputWithFrameAdjustment:]
I did not modify any of these call sites. -[layoutSubviews] passes "NO" to
layoutTokensAndInputWithFrameAdjustment, and that was the boolean I needed
to prevent clearing the inputTextField.text.

It seems unlikely that existing clients would be relying on the text field
being cleared in response to layoutSubviews. If you know for a fact that
there are clients, I can add a property to clear/not-clear the textField on
layoutSubviews. (Making the default be "do not clear"-- old behavior)
Would that make you more comfortable?

Thank you again for your time
Ray


Reply to this email directly or view it on GitHub
#77 (comment).

@ayanonagon
Copy link
Contributor

Realized I can merge PR’s from my phone. Thanks again! :octocat:

ayanonagon added a commit that referenced this pull request Sep 11, 2015
Clear input when we are adjusting frame
@ayanonagon ayanonagon merged commit 7586ae8 into venmo:master Sep 11, 2015
@walsh2000
Copy link
Author

Excellent! Thank you looking into this one.

@walsh2000
Copy link
Author

Do you think you'll bump the podspec? Or should I pin to that revision?

@ayanonagon
Copy link
Contributor

Hi @walsh2000! Sorry for the delay. We just released 2.5.1 so you should be able to point to that now. Thanks again fro your PR :D

@walsh2000
Copy link
Author

@ayanonagon

Good deal! Thank you very much.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants