[TM] Add spec for AlertManager #24906
Conversation
facebook-github-bot
added
the
CLA Signed
react-native-bot
added
API: Alert
Type: Enhancement
| type Button = {| | ||
| text: string, | ||
| onPress: () => void, | ||
| style: 'default' | 'cancel' | 'destructive', |
There was a problem hiding this comment.
Unfortunately, our Codegen doesn't support unions. 😔
| 'plain-text': string, | ||
| 'secure-text': string, | ||
| 'login-password': string, | ||
| }>; |
There was a problem hiding this comment.
It also doesn't support enums. 😔
|
@RSNara Thanks for reviewing. 👍 |
| @@ -10,6 +10,6 @@ | |||
|
|
|||
| 'use strict'; | |||
|
|
|||
| const RCTAlertManager = require('../BatchedBridge/NativeModules').AlertManager; | |||
| const RCTAlertManager = require('./NativeAlertManager'); | |||
sasurau4
force-pushed
the
spec-alertmanager
branch
from
fbceb18
to
2b5b22f
Compare
|
@uqmessias Thanks for reviewing 👍 |
| ) => void; | ||
| } | ||
|
|
||
| export default TurboModuleRegistry.getEnforcing<Spec>('AlertManager'); |
There was a problem hiding this comment.
Since, this is an iOS only feature, please export it conditionally.
|
It's almost there, just left a few comments |
Libraries/Alert/Alert.js
Outdated
| destructive: string, | ||
| }>; | ||
| /* 'default' | 'cancel' | 'destructive' */ | ||
| export type AlertButtonStyle = string; |
There was a problem hiding this comment.
Good job!
But it would be better to keep this inside the spec file, because there is where we're defining types. We're could still exporting it here too or updating all call-sites that uses these types from here to the spec path.
What do you think about it?
There was a problem hiding this comment.
I agree with keeping types inside the spec file. I'll move types to spec file 👍
There was a problem hiding this comment.
@fkgozali has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
@uqmessias Thanks for comments! I worked on it! Plz review 🙏 |
There was a problem hiding this comment.
@fkgozali has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
| type AlertType, | ||
| type AlertButtonStyle, | ||
| } from './NativeAlertManager'; | ||
| import {type Buttons, type Options, type AlertType} from './NativeAlertManager'; |
There was a problem hiding this comment.
This is a minor and no need to update it, but you also can use
import type { Buttons, Options, AlertType } from './NativeAlertManager';
|
We still need @RSNara's approval |
|
This pull request was successfully merged by @sasurau4 in 122cc8b. When will my fix make it into a release? | Upcoming Releases |
Summary: Part of facebook#24875 ## Changelog [General] [Added] - Add TurboModule spec for AlertManager Pull Request resolved: facebook#24906 Reviewed By: lunaleaps Differential Revision: D15471065 Pulled By: fkgozali fbshipit-source-id: bb22e6454b1f748987f3a8cd957bfd4e027493a5
Summary
Part of #24875
Changelog
[General] [Added] - Add TurboModule spec for AlertManager
Test Plan
yarn flow