Skip to content

[TM] Add spec for DialogManagerAndroid #24912

Closed
uqmessias wants to merge 2 commits into
facebook:masterfrom
uqmessias:master
Closed

[TM] Add spec for DialogManagerAndroid #24912
uqmessias wants to merge 2 commits into
facebook:masterfrom
uqmessias:master

Conversation

@uqmessias
Copy link
Copy Markdown
Contributor

Summary

Part of #24875.

Changelog

[General] [Added] - TM add spec for DialogManagerAndroid

Test Plan

Flow green.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label May 16, 2019
@react-native-bot react-native-bot added Platform: Android Android applications. Type: Enhancement A new feature or enhancement of an existing feature. labels May 16, 2019
@uqmessias uqmessias force-pushed the master branch 3 times, most recently from c2325b8 to ba573b5 Compare May 16, 2019 21:13
@ericlewis ericlewis added the Flow label May 16, 2019
@uqmessias uqmessias force-pushed the master branch 2 times, most recently from 19ee97f to c8e824d Compare May 16, 2019 21:56
Copy link
Copy Markdown
Contributor

@RSNara RSNara left a comment

Choose a reason for hiding this comment

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

Looks mostly good! 😁
However, I do have a few questions/concerns. Could you look through my comments?

Comment thread Libraries/NativeModules/specs/NativeDialogManagerAndroid.js Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Unfortunately, the Codegen system doesn't support unions. 😔

So, I think it's best if we change this to string.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

What about keeping this as it is and adding and updating DialogAction to type DialogAction = string?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looking through the code, it seems like onAction can also be invoked without the buttonKey:

    @Override
    public void onDismiss(DialogInterface dialog) {
      if (!mCallbackConsumed) {
        if (getReactApplicationContext().hasActiveCatalystInstance()) {
          mCallback.invoke(ACTION_DISMISSED);
          mCallbackConsumed = true;
        }
      }
    }

So we should make this argument nullable.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@RSNara, I saw one of your comments in another PR about the codegen not supporting enum or union types. So, based on that, these two types (DialogAction and DialogButtonKey) are not supported by the codegen, right?

Copy link
Copy Markdown

@analysis-bot analysis-bot left a comment

Choose a reason for hiding this comment

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

Code analysis results:

  • eslint found some issues. Run yarn lint --fix to automatically fix problems.

Comment thread Libraries/NativeModules/specs/NativeDialogManagerAndroid.js Outdated
@uqmessias uqmessias force-pushed the master branch 3 times, most recently from 95a8aae to ba8d9f2 Compare May 17, 2019 22:03
@uqmessias
Copy link
Copy Markdown
Contributor Author

@RSNara,
Can you take a look on this again, pls?

Copy link
Copy Markdown
Contributor

@RSNara RSNara left a comment

Choose a reason for hiding this comment

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

Looks good! Thank you for iterating on this. 😁

Copy link
Copy Markdown
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@RSNara has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@react-native-bot
Copy link
Copy Markdown
Collaborator

This pull request was successfully merged by @uqmessias in 32340d3.

When will my fix make it into a release? | Upcoming Releases

@react-native-bot react-native-bot added the Merged This PR has been merged. label May 23, 2019
M-i-k-e-l pushed a commit to M-i-k-e-l/react-native that referenced this pull request Mar 10, 2020
Summary:
Part of facebook#24875.

## Changelog

[General] [Added] - TM add spec for DialogManagerAndroid
Pull Request resolved: facebook#24912

Reviewed By: fkgozali

Differential Revision: D15433854

Pulled By: RSNara

fbshipit-source-id: e7234debe16de5afbc770f8feee2471f41b54427
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Flow Merged This PR has been merged. Native Module Platform: Android Android applications. Type: Enhancement A new feature or enhancement of an existing feature.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants