-
Notifications
You must be signed in to change notification settings - Fork 24.3k
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
[TM] Add spec for AccessibilityManager #24894
[TM] Add spec for AccessibilityManager #24894
Conversation
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.
looks great! There are some types missing from the android side of things however:
Lines 102 to 110 in 09f17a4
@ReactMethod | |
public void isReduceMotionEnabled(Callback successCallback) { | |
successCallback.invoke(mReduceMotionEnabled); | |
} | |
@ReactMethod | |
public void isTouchExplorationEnabled(Callback successCallback) { | |
successCallback.invoke(mTouchExplorationEnabled); | |
} |
Libraries/Components/AccessibilityInfo/AccessibilityInfo.ios.js
Outdated
Show resolved
Hide resolved
Libraries/Components/AccessibilityInfo/NativeAccessibilityManager.js
Outdated
Show resolved
Hide resolved
+removeListeners: (eventName: string, handler: Function) => void; | ||
} | ||
|
||
export default TurboModuleRegistry.getEnforcing<Spec>('AccessibilityManager'); |
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.
the android side of this is named AccessibilityInfo
use platform here to serve up either:
Platform.OS === 'ios' ? TurboModuleRegistry.getEnforcing<Spec>('AccessibilityManager') : TurboModuleRegistry.getEnforcing<Spec>('AccessibilityInfo')
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.
They're going to have a bit different API, that's why they live separate files.
I think:
Platform.OS === 'ios' ? TurboModuleRegistry.getEnforcing<Spec>('AccessibilityManager') : null
makes more sense
I think this and |
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.
@fkgozali has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
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.
LGTM, just a minor change for the conditional.
} | ||
|
||
export default TurboModuleRegistry.getEnforcing<Spec>('AccessibilityManager'); | ||
export default (Platform.OS === 'ios' | ||
? TurboModuleRegistry.getEnforcing<Spec>('AccessibilityManager') |
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.
remove this conditional, and just use get
This pull request was successfully merged by @michalchudziak in 71461cb. When will my fix make it into a release? | Upcoming Releases |
Summary: Part of facebook#24875 ## Changelog [General] [Added] - Add TurboModule spec for AccessibilityManager Pull Request resolved: facebook#24894 Reviewed By: rickhanlonii Differential Revision: D15471243 Pulled By: fkgozali fbshipit-source-id: 33f39d41d70da9380f29f2eb47e8c7682b323030
Summary
Part of #24875
Changelog
[General] [Added] - Add TurboModule spec for AccessibilityManager
Test Plan
Run
yarn flow
on the root level