Skip to content
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

Support alert messages customization [ios] #38

Merged
merged 3 commits into from
Sep 28, 2021

Conversation

Undermaken
Copy link
Contributor

@Undermaken Undermaken commented Sep 24, 2021

This PR updates the iOS compiled framework that includes these changes
pagopa/io-cie-ios-sdk#8

In addition it adds all ReactNative stuffs to make the communication between JS and the native side

how to test

You can test it with app IO directly

  • on IOapp set this branch for react-native-cie
    from
    "@pagopa/react-native-cie": "^1.1.2",
    to
    "@pagopa/react-native-cie": "git://github.com/pagopa/io-cie-sdk.git#support-ios-alert-message-customization"
  • yarn cie-ios:prod
  • use a real device
  • open xCode and run play
  • customize alert messages
  public async startCieiOS() {
    cieManager.removeAllListeners();
    cieManager.onEvent(this.handleCieEvent);
    cieManager.onError(this.handleCieError);
    cieManager.onSuccess(this.handleCieSuccess);
    👇
    cieManager.setAlertMessage("readingInstructions", "test React Native");
    cieManager.setAlertMessage(
      "wrongPin2AttemptLeft",
      "hai sbagliato il PIN riprova per favore"
    );
    await cieManager.setPin(this.ciePin);
    cieManager.setAuthenticationUrl(this.cieAuthorizationUri);
    cieManager
      .start()
      .then(async () => {
        await cieManager.startListeningNFC();
        this.setState({ readingState: ReadingState.waiting_card });
      })
      .catch(() => {
        this.setState({ readingState: ReadingState.error });
      });
  }

@Undermaken Undermaken changed the title Add the support to customize alert messages Support to customize alert messages Sep 24, 2021
@Undermaken Undermaken changed the title Support to customize alert messages Support alert messages customization [ios] Sep 24, 2021
@pagopa-github-bot
Copy link
Collaborator

pagopa-github-bot commented Sep 24, 2021

Warnings
⚠️ Please include a Pivotal story at the beginning of the PR title (see below).

Example of PR titles that include pivotal stories:

  • single story: [#123456] my PR title
  • multiple stories: [#123456,#123457,#123458] my PR title

Generated by 🚫 dangerJS against 8e3bb8d

* only iOS.
* customize the info and error messages shown on NFC reading card alert
*/
setAlertMessage(key: iOSAlertMessageKeys, value: string): void;
Copy link
Contributor

@fabriziofff fabriziofff Sep 27, 2021

Choose a reason for hiding this comment

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

What you think about change this method as:

  setAlertMessages(ReadonlyArray<{key: iOSAlertMessageKeys, value: string}>): void;

in order to define the configuration of all the possible alert messages one time and pass in a single function instead of using multiple functions?

Not releated to this pr, but in general I think could improve the usage and readability of the code define the configuration of the library and pass it in the start method, without using multiple methods, eg:

type CieConfiguration = {
pin: string;
authUrl: string;
iosAlertMessage?: ReadonlyArray<key: iOSAlertMessageKeys, value: string>;
onEvent?: (event: Event) => void;
onSuccess?: (url: string) => void
onError?: (error: Error) => void
};

const cieConfig: CieConfiguration = {...};
cieManager.start(cieConfig);

Copy link
Contributor Author

@Undermaken Undermaken Sep 27, 2021

Choose a reason for hiding this comment

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

I'm agree, your suggestions are an improvement.
So I made a commit to address both

  1. Now you can start the SDK by passing the alert messages configuration. It has effect only on iOS. I prefer a partial record instead of array, to avoid items duplication. i.e
cieManager.start({ readingInstructions: "my test" })
  1. I kept setAlertMessage so we can change an alert message also when the SDK is already started

c39b05f

@fabriziofff fabriziofff merged commit df251fe into master Sep 28, 2021
@fabriziofff fabriziofff deleted the support-ios-alert-message-customization branch September 28, 2021 08:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants