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

fix: delegate should be set on nativeTextViewProtected #8881

Merged
merged 1 commit into from Sep 23, 2020

Conversation

farfromrefug
Copy link
Collaborator

just like textfields. Fixes components extending this one

PR Checklist

What is the current behavior?

What is the new behavior?

Fixes/Implements/Closes #[Issue Number].

just like textfields. Fixes components extending this one
@cla-bot cla-bot bot added the cla: yes label Sep 22, 2020
@NathanaelA NathanaelA changed the title delegate should be set on nativeTextViewProtected delegate should be set on nativeTextViewProtected - BROKEN DO NO MERGE Sep 22, 2020
Copy link
Contributor

@NathanaelA NathanaelA left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting -- I have to be honest, having nativeTextViewProtected confuses me, and I was thinking maybe either something legacy, or potentially a extended class might have it own separate nativeTextViewProtected.

So I just scanned the entire NS source code for nativeTextViewProtected to see if their is somewhere that this is actually using a variable and not just the textbase.getter. It is used 178 times in the code; however ALL of them extend text-base-common so they all are using the getter that returns nativeViewProtected as the value that is being changed.

It is used in:
Label, editable-text-base, text-view, text-field, and finally text-base. However all of them extend text-base; which is where they get access to the getter nativeTextViewProtected which is just a getter that returns nativeViewProtected

Can anyone think of a good reason to not rename all instances of nativeTextViewProtected back to nativeViewProtected to simplify the codebase? (However leaving the nativeTextViewProtected getter so that we don't break any third party plugin code...)

Please note nativeViewProtected is used by View which is one of the base classes ALL views extend from, so having the name change in a few of the components but be still mapped to the same base value seems confusing to me...

Edit: this comment makes more sense AFTER the review -- it was done after the review; so realize that this is after I made comments on the code changes..

packages/core/ui/text-view/index.ios.ts Show resolved Hide resolved
packages/core/ui/text-view/index.ios.ts Show resolved Hide resolved
@rigor789 rigor789 marked this pull request as draft September 22, 2020 17:03
@NathanaelA NathanaelA changed the title delegate should be set on nativeTextViewProtected - BROKEN DO NO MERGE delegate should be set on nativeTextViewProtected - Do not merge Sep 22, 2020
@farfromrefug
Copy link
Collaborator Author

Please dont do that. That was done in a effort with the old N team to make it work with material native plugins where a text field/textview is made of 2 views : the layout and the actual text field. So some properties must be applied to one and some to the other. This brings a lot of flexibility to plugins developers.

By changing that you will break all my material plugins !
That PR simply finishes the work we started with the old team (textview was not in scope at the time)

@NathanaelA

This comment was marked as abuse.

@farfromrefug
Copy link
Collaborator Author

I told you it helps in separating what should be applied to the textview and what should be applied to the layout.

And of course you will break plugins if you remove it !
btw this PR / fix is needed to fix issue users are having because the textview is not behaving as the text field.

@NathanaelA

This comment was marked as abuse.

@farfromrefug
Copy link
Collaborator Author

farfromrefug commented Sep 23, 2020

Really bold, uppercase? Seriously....

It fixes delegate being set on the wrong view on iOS (the layout and not the view).if
I can't show code it fixes in my comp.as it is not code in my comp but in N. For.my comp nativeViewProtected and nativeTextViewProtected are different. Before this PR the delegate for UITextView was set on the "layout" view (represented by nativeViewProtected)
You should read this
#6102
#8514
EDIT: it does fix an issue in N. In the approach used in N with nativeTextViewProtected, the delegate was set on the wrong view. No "visual side effect" in N ui but the code is wrong and breaks plugins based on this

@NathanaelA

This comment was marked as abuse.

@NathanaelA

This comment was marked as abuse.

@NathanaelA NathanaelA self-requested a review September 23, 2020 22:33
@NathanaelA NathanaelA changed the title delegate should be set on nativeTextViewProtected - Do not merge fix: delegate should be set on nativeTextViewProtected Sep 23, 2020
@NathanaelA NathanaelA marked this pull request as ready for review September 23, 2020 22:34
Copy link
Contributor

@NathanaelA NathanaelA left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on the code Igor linked me to; I believe this is a valid use case to eliminate extra work on the plugin author without being a major issue for the framework. We do need to document this getter and we created a PR #8886 and any comments would be helpful

@NathanaelA NathanaelA merged commit 13decc4 into master Sep 23, 2020
@NathanaelA NathanaelA deleted the farfromrefug-patch-4 branch September 23, 2020 23:14
NathanWalker pushed a commit that referenced this pull request Sep 23, 2020
just like textfields. Fixes components extending this one
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants