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

Providing multiple errors in one alertWithTyp() function call throws a warning #115

Closed
VicFrolov opened this issue Feb 15, 2018 · 8 comments

Comments

@VicFrolov
Copy link

Short Description

I've noticed that if I pass an array of errors, e.g. ["password field is blank", "password fields do not match"], it works perfectly, as I set the numOfLines to the length of the array, and it shows the data appropriately.

However, I am getting a warning "DropdownAlert: Message is not a string". I wanted to know if I should be passing the data differently, or if this requires a PR, and an array of strings is indeed valid.

Steps to Reproduce / Code Snippets / Usage

With the example source code, change the alertWithType line to:

this.dropdown.alertWithType('error', 'Error', ['Error 1', 'Error2', 'Error3');

and

      <DropdownAlert
        ref={ref => (this.dropdown = ref)}
        messageNumOfLines={3}
      />

Expected Results

No Warning being thrown

Additional Information

  • React Native version: 0.52.0
  • react-native-dropdownalert version: 3.3.0
  • Platform(s) (iOS, Android, or both?): ios
  • iOS version: ios 11
  • Android version: N/A
@testshallpass
Copy link
Owner

hi @VicFrolov the title and message are converted to strings and a warning is logged out. These parameters are expected to be string types. I suppose the warning could be suppressed if the parameters can be converted. If they can't be converted then an warning should appear and fails displaying the alert.

@VicFrolov
Copy link
Author

Got it, how would you recommend passing multiple errors, where each error is on a separate line? With an array, it does this for me for free, otherwise the error looks instead of:

First Error
Second Error
Third Error

like this:
First Error, Second Error, Third Error

@testshallpass
Copy link
Owner

I haven't tried it but you can maybe get away with first error\nsecond error\nthird error and set number of lines to 0 (unlimited for iOS)

@VicFrolov
Copy link
Author

VicFrolov commented Feb 19, 2018

I think you wold agree that this wouldn't be an optimal approach, and should be done under the hood. I personally believe (and could be wrong) that a more intuitive approach is to allow passing in an array of errors, where the number of lines is the length of the array. I'd be happy to make a PR if you agree with this perspective.

The down side of course is passing an error as a simple string vs an array with one item is more intuitive. Having said that, I still find an array of n amounts of errors that defaults numberOfLines to be a superior implementation.

@testshallpass
Copy link
Owner

I am going to leverage the discussion and make an enhancement to the title and message parameters. Basically, in a way to be more defensive and flexible.

@testshallpass testshallpass moved this from backlog to wishlist in react-native-dropdownalert Sep 30, 2018
@testshallpass testshallpass moved this from wishlist to backlog in react-native-dropdownalert Oct 4, 2018
@testshallpass testshallpass moved this from backlog to wishlist in react-native-dropdownalert Oct 4, 2018
@testshallpass testshallpass moved this from wishlist to backlog in react-native-dropdownalert Oct 4, 2018
@testshallpass
Copy link
Owner

Acceptance criteria

  • Support array of messages.
  • One message per line.
  • array.length is fed to numberOfLines for Text prop.
  • messageNumberOfLines overridden if message array is present.
  • Warn if message(s) can not be converted to strings.
  • Backwards compatibility with message as an object (most likely a string).

@testshallpass
Copy link
Owner

@VicFrolov OK so I ran into an issue. I was testing an array of messages like:

const messages = ['Do laborum ipsum deserunt laboris fugiat fugiat deserunt Lorem esse culpa exercitation est dolore deserunt.', 'Non magna nulla incididunt ullamco sint incididunt Lorem qui Lorem anim ullamco pariatur aliquip sunt.', 'Enim eu excepteur laborum eu magna ea exercitation veniam exercitation reprehenderit.'];

I then separate messages to new lines with messages.join('\n'); It resulted in numberOfLines = 3 but in reality should be 8 to fully display all messages. I am going to rethink approach.

@testshallpass testshallpass moved this from doing to todo in react-native-dropdownalert Feb 8, 2019
@testshallpass
Copy link
Owner

I'm closing based on previous comment and no bandwidth to work on it. In 4.0.0 you can provide array as title and message that is converted to a joined string (see: getStringValue), however it does not automatically adjust numberOfLines which can be configured on the implementer side. For example, numberOfLines set to 0 will be unlimited on iOS and set it to big number on Android and should be okay. Plus. if you want to create newlines you can add \n to title or message, now I haven't tried it but I assume it works as expected.

react-native-dropdownalert automation moved this from todo to Done Jun 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

No branches or pull requests

2 participants