-
-
Notifications
You must be signed in to change notification settings - Fork 643
refactor(tabs): replace isNativeAction with actionOrigin enum #3949
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 3 commits
4615c77
a98d394
ae93ced
b62d8d2
c10e5ad
7e5a868
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 |
|---|---|---|
| @@ -0,0 +1,25 @@ | ||
| package com.swmansion.rnscreens.gamma.tabs.container | ||
|
|
||
| import com.swmansion.rnscreens.gamma.tabs.container.TabsActionOrigin.PROGRAMMATIC_JS | ||
| import com.swmansion.rnscreens.gamma.tabs.container.TabsActionOrigin.USER | ||
|
|
||
| /** | ||
| * Origin (actor) that requested a tab transition. Mirrors the public `actionOrigin` event field. | ||
| * | ||
| * - [USER] — direct native UI interaction (tab bar tap). | ||
| * - [PROGRAMMATIC_JS] — JS-initiated request delivered via the `navStateRequest` prop. | ||
| * | ||
| * The `implicit` origin defined on the public TS API is iOS-only at the moment; | ||
| * Android does not currently produce it. | ||
| */ | ||
| enum class TabsActionOrigin { | ||
| USER, | ||
| PROGRAMMATIC_JS, | ||
| ; | ||
|
|
||
| override fun toString(): String = | ||
| when (this) { | ||
| USER -> "user" | ||
| PROGRAMMATIC_JS -> "programmatic-js" | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -62,10 +62,16 @@ export type TabSelectedEvent = { | |
| /** Whether the selection triggered a special effect (e.g. scroll-to-top on repeated selection). */ | ||
| hasTriggeredSpecialEffect: boolean; | ||
| /** | ||
| * False in case the event is a result of JS-driven update. True otherwise, e.g. in case of user action (tap) | ||
| * or implicit UIKit action (app resize, orientation change, etc.). | ||
| * @summary Origin (actor) that requested this tab transition. | ||
| * | ||
| * @description | ||
| * - `'user'` — direct native UI interaction (e.g. tab bar tap, iOS tab drag-and-drop). | ||
|
Contributor
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. I don't think I like
Member
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. Good remark, but I see it the other way around. All of those should describe WHO (the origin) triggered that, and not how. But I don't have better naming ideas for the other enum variants. Do you happen to have any suggestions? I'll proceed here & refactor it later if we come up with some nice naming here. |
||
| * - `'programmatic-js'` — JS-initiated request delivered via the `navStateRequest` prop. | ||
| * - `'implicit'` — platform side effect not attributable to an explicit actor | ||
| * (e.g. UIKit reshuffling the selection during a horizontal size-class transition on iPad). | ||
| * Currently only emitted on iOS. | ||
|
kligarski marked this conversation as resolved.
Outdated
|
||
| */ | ||
| isNativeAction: boolean; | ||
| actionOrigin: 'user' | 'programmatic-js' | 'implicit'; | ||
| }; | ||
|
|
||
| /** | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not needed, this file defines these
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At the first glance I agree, but when I run "optimise imports" action in this file - it does nothing - it qualifies these as used.
I've removed these and the app still builds successfully -> leaving them removed, as you suggested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
7e5a868