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

Add new property alwaysHandleTapTruncationAction to ASTextNode2 and ASTextNode. #1520

Merged
merged 7 commits into from
Jul 30, 2019

Conversation

dirtmelon
Copy link
Contributor

After this PR, ASTextNode and ASTextNode2 don't handle touches on additional attributed message if passthroughNonlinkTouches is YES. But sometimes we maybe want to handle these touches action, even if passthroughNonlinkTouches is YES.

So I add the new property alwaysHandleTapTruncationAction to ASTextNode and ASTextNode2, when alwaysHandleTapTruncationAction is YES, ASTextNode and ASTextNode2 always handle touches on additional attributed message.

@dirtmelon
Copy link
Contributor Author

Could you please help to review this pr? Many thanks. @maicki @Adlai-Holler @garrettmoon @appleguy @nguyenhuy @bolsinga

@Adlai-Holler
Copy link
Member

I can't remember why we decided not to call the delegate method if passthroughNonlinkTouchesEnabled again? I reviewed the original PR but it's not clear why that delegate call is a problem. @maicki What do you think of this diff?

@dirtmelon
Copy link
Contributor Author

I found this issue, possibly because of this.Sometimes we don't want to handle these touch actions. @Adlai-Holler @maicki

@dirtmelon
Copy link
Contributor Author

@nguyenhuy @Adlai-Holler Please review if possible.

@nguyenhuy
Copy link
Member

@maicki ping

@maicki
Copy link
Contributor

maicki commented Jun 28, 2019

@dirtmelon Thanks for the PR!

I'm good with the change. Maybe let's use a different name that is more aligned with the callback method that is called like: alwaysHandleTruncationTokenTap and add as documentation to the property that if this is set to YES, the --[ASTextNodeDelegate textNodeTappedTruncationToken:]` callback will be called.

@dirtmelon
Copy link
Contributor Author

@maicki I have renamed it, please review it again.

@ghost
Copy link

ghost commented Jun 28, 2019

🚫 CI failed with log

@dirtmelon
Copy link
Contributor Author

@nguyenhuy Could you please review it?

Source/ASTextNode.mm Outdated Show resolved Hide resolved
@nguyenhuy
Copy link
Member

Also, can you please rebase with latest master to fix the CI issue?

Source/ASTextNode.mm Outdated Show resolved Hide resolved
Source/ASTextNode.mm Show resolved Hide resolved
Copy link
Member

@nguyenhuy nguyenhuy left a comment

Choose a reason for hiding this comment

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

LGTM now. Thanks for the contribution!

@nguyenhuy nguyenhuy merged commit a88e3cf into TextureGroup:master Jul 30, 2019
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.

None yet

4 participants