-
-
Notifications
You must be signed in to change notification settings - Fork 611
feat(Tabs): special effects refactor #3440
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
Changes from all commits
83c4518
d54e909
3e61082
92a7d15
efea3cc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,25 +9,36 @@ class TabsHostNativeFocusChangeEvent( | |
| surfaceId: Int, | ||
| viewId: Int, | ||
| val tabKey: String, | ||
| val tabNumber: Int, | ||
| val repeatedSelectionHandledBySpecialEffect: Boolean, | ||
| ) : Event<TabScreenDidAppearEvent>(surfaceId, viewId), | ||
| NamingAwareEventType { | ||
| override fun getEventName() = EVENT_NAME | ||
|
|
||
| override fun getEventRegistrationName() = EVENT_REGISTRATION_NAME | ||
|
|
||
| // All events for a given view can be coalesced. | ||
| override fun getCoalescingKey(): Short = 0 | ||
| // If the user taps currently selected tab 2 times and e.g. scroll to top effect can run, | ||
| // we should send 2 events [(tabKey, true), (tabKey, false)]. We don't want them to be coalesced | ||
| // as we would lose information about activation of special effect. That's why we take into | ||
| // account `repeatedSelectionHandledBySpecialEffect` for coalescingKey. | ||
| override fun getCoalescingKey(): Short = (tabNumber * 10 + if (repeatedSelectionHandledBySpecialEffect) 1 else 0).toShort() | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's the reasoning here? Why different key when FYI: I don't think this works anymore, as recently discovered by @t0maboro.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If the user taps currently selected tab 2 times and e.g. scroll to top effect can run, we should send [(tabKey, true), (tabKey, false)]. If I understand correctly, coalescing key is used to combine events to prevent spamming them. If those 2 events were combined to only one, we would lose some information that somebody can rely on (e.g. running some action at the same time as special effect or after special effect).
Just wanted to make sure that it works correctly when it gets fixed 😅
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This convinces me. I think this deserves code comment. Let's make one, please.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
|
|
||
| override fun getEventData(): WritableMap? = | ||
| Arguments.createMap().apply { | ||
| putString(EVENT_KEY_TAB_KEY, tabKey) | ||
| putBoolean( | ||
| EVENT_KEY_REPEATED_SELECTION_HANDLED_BY_SPECIAL_EFFECT, | ||
| repeatedSelectionHandledBySpecialEffect, | ||
| ) | ||
| } | ||
|
|
||
| companion object : NamingAwareEventType { | ||
| const val EVENT_NAME = "topNativeFocusChange" | ||
| const val EVENT_REGISTRATION_NAME = "onNativeFocusChange" | ||
|
|
||
| private const val EVENT_KEY_TAB_KEY = "tabKey" | ||
| private const val EVENT_KEY_REPEATED_SELECTION_HANDLED_BY_SPECIAL_EFFECT = | ||
| "repeatedSelectionHandledBySpecialEffect" | ||
|
|
||
| override fun getEventName() = EVENT_NAME | ||
|
|
||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.