added alert api #305

Open
wants to merge 3 commits into
from

Projects

None yet

2 participants

@vaukalak
Contributor
vaukalak commented Dec 30, 2016 edited

This patch solves the following problem

When developing mobile application, there a lot of cases when Alert api is used. Anyway it's not available in react-native-web and developer have to rewrite a lot of code. This patch solves the issue.

Test plan

Added to storie book.

This pull request

  • includes documentation
  • includes tests
  • includes an interactive example
  • includes screenshots/videos
@vaukalak vaukalak referenced this pull request Jan 2, 2017
Closed

Adding an Alert API #143

@necolas
Owner
necolas commented Jan 4, 2017

Thank you! I'll get around to reviewing this soon

@necolas

I'd prefer the default style to be Android for now.

There's also an API for making the alert view cancellable when clicking on the background.

+By default it'll look as ios alert, anyway there's possibility to style it.
+## Methods
+
+static **setStyle**(config: Object)
@necolas
necolas Jan 9, 2017 Owner

Please remove the setStyle API and document alert

@vaukalak
vaukalak Jan 10, 2017 Contributor

@necolas why don't you like setStyle API? the app can run, for example, on ios/web platforms in this case alert similar to android native alert will not make a lot of sense. I think it could be a good idea to export STYLE_ANDROID and STYLE_IOS. And keep possibility to switch to desired style.. BTW, android alerts has different styles from version to version, could you please point me to the image of alert you'd like to have?

src/apis/Alert/index.js
+
+let instance;
+
+let styles = {
@necolas
necolas Jan 9, 2017 Owner

styles should be registered with StyleSheet.create

src/apis/Alert/index.js
+
+let styles = {
+ background: {
+ position: 'absolute',
@necolas
necolas Jan 9, 2017 Owner

You can spread StyleSheet.absoluteFillObject instead

src/apis/Alert/index.js
+ render() {
+ const { visible, message, title, buttons } = this.state;
+ if (!visible) {
+ return <View />;
@necolas
necolas Jan 9, 2017 Owner

should be able to return null here

+ {
+ buttons.map(
+ ({ text, onPress }, i) => (
+ <TouchableOpacity
@necolas
necolas Jan 9, 2017 Owner

In terms of accessibility, focus needs to move to the first button when the alert opens and return to the previously focused element after it closes

@vaukalak
vaukalak Jan 10, 2017 Contributor

@necolas Not sure, how to implement it..

@vaukalak
Contributor

@necolas
1 - what should we do with the setStyle api? I think this feature would be very use full in web. On mobile platforms RN displays native alert, which are intuitive for users, on web, it should be probably allowed, to show users the alert which follows general application style.
2 - not sure where to indicate api to close on background press: can't find anything related in the doc
3 - not sure what do you mean by focusing on the first button.
4 - need a screenshot of android alert you'd like to be as default style.
5 - after update test are successful on my mac, but fails on travis, can't understand the reason from the log.

thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment