Alert docs & binding adaptions

[fhammerschmidt]

I wanted to write a blogpost about zero-cost bindings and take RN as an example. I compared the react-native sources to our bindings and found an error - there is no type field (anymore). They also mention the type field in the docs - so I sent them a PR too.

I took this opportunity to write a little documentation about Alert.

[sgny]

Thanks for drawing attention to this, but it is (sadly) a bit more complicated.

Looking at the source for Alert, I see that they have kept that prop on iOS and it actually works.

[fhammerschmidt]

Well, are you sure? If you look at the current master on iOS they use Alert.prompt with "default" as the alert type - no type gets passed through.

[sgny]

Yes, I tested.

[MoOx]

Does this need any update? Should this be in incoming latest 0.60 patch release?

[fhammerschmidt]

I checked the sources where alertType is not used and submitted a PR to the React-Native docs to delete them. They merged it, so I would say you can incorporate this PR as-is.

[sgny]

Sorry no ways around it, this is simply incorrect based on the RN master.

[fhammerschmidt]

I am too dumb then. I guess it is because they merged AlertIOS and Alert?
So we need to type this one: https://github.com/facebook/react-native/blob/baa66f63d8af2b772dea8ff8eda50eba264c3faf/Libraries/Alert/NativeAlertManager.js#L16 ?

@sgny

[sgny]

I didn't intend that to be so heavy handed, my apologies.

I think we're miscommunicating. Seems to be mostly my fault because I was thinking of AlertIOS at the same time and accordingly focused on alertType which is used with prompt but I suppose your PR is quite clear that you are addressing only alert, not prompt. Then you are perfectly right.

I should have been clearer what I thought was wrong with it (Alert and AlertIOS needs to be merged and then alertType is still needed), so once again sorry about that.

[fhammerschmidt]

So what should I do? I can type prompt as well, but it's not even mentioned in the docs.
But I think the guys over at RN need to move that from AlertIOS (which already is flagged as deprecated).

[sgny]

prompt has already been added to Alert and AlertIOS is gone. Since AlertIOS would (rightfully) be removed from these bindings with 0.61, I would like to get prompt included soon-ish but I can take care of that.

So it seems we can merge this and my apologies for not realising you weren't merging AlertIOS with this in the first place.

[fhammerschmidt]

I will update this PR with prompt bindings in a couple of hours when I have the time.

[fhammerschmidt]

Done. But I did not document the prompt method (as it is the case in the RN docs).

[sgny]

Perhaps these can be moved to a new section called Types

Something along the lines of an entry for the type and the signature of the constructor:

## `options`

options:
  (~cancelable: bool=?, ~onDismiss: unit => unit=?, unit) => options

[fhammerschmidt]

I am not sure what you mean? A separate Module or even File for the options and buttons types and externals? Can you point me to a binding where this is already the case?

[sgny]

As above, should be moved to a Types section.

[sgny]

Thanks, nice use of bs.unwrap 👍

We can work on the documentation later.

[fhammerschmidt]

@MoOx can we merge this? I think everything else can also be adapted at a later stage.

[MoOx]

I am afk until Monday, I will try to review all the pending pr next week and cut some releases.

[sgny]

@MoOx, shall we create a 0.61 branch and try to get a few more commits into that?

I will try to work on the ScrollView scrollTo* functions issue (#604) before the next release. That has been open for some time as a known bug (even if it is due to poor RN documentation).

[MoOx]

@sgny I am preparing the 0.60.x release so we can move forward with a single branch, unless we really need to have separate one