venmo / VENTokenField

Easy-to-use token field that is used in the Venmo app.
MIT License
795 stars 195 forks source link

Clear input when we are adjusting frame #77

Closed walsh2000 closed 9 years ago

walsh2000 commented 9 years ago

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.

walsh2000 commented 9 years ago

Has anyone had a moment to consider this PR?

walsh2000 commented 9 years ago

@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 commented 9 years ago

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 commented 9 years ago

@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 commented 9 years ago

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 notifications@github.com 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 https://github.com/venmo/VENTokenField/pull/77#issuecomment-138686427.

ayanonagon commented 9 years ago

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

walsh2000 commented 9 years ago

Excellent! Thank you looking into this one.

walsh2000 commented 9 years ago

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

ayanonagon commented 8 years ago

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 commented 8 years ago

@ayanonagon

Good deal! Thank you very much.