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

react-native facades #214

Closed
wants to merge 1 commit into from
Closed

react-native facades #214

wants to merge 1 commit into from

Conversation

dohrayme
Copy link

@dohrayme dohrayme commented Jan 2, 2019

some additional react-native facades I've found useful

Copy link
Owner

@shadaj shadaj left a comment

Choose a reason for hiding this comment

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

Just one props typing adjustment needed, otherwise looks good!

import scala.scalajs.js.annotation.JSImport

@react object TouchableHighlight extends ExternalComponent {
case class Props(onPress: () => Unit,
Copy link
Owner

Choose a reason for hiding this comment

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

Looks like onPress is optional according to the docs, so this should be js.UndefOr[() => Unit]. Same for TouchableOpacity.

Copy link
Author

Choose a reason for hiding this comment

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

That is true, but is it useful to have a Touchable component without an onPress action? Don't we lose some typesafety by making it optional?

Copy link
Owner

Choose a reason for hiding this comment

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

My general approach is to match what the docs say, and in this case the onPress is marked as optional, so I would prefer having the type here also be optional so that anything that works in JS also works in Slinky.

For typesafety, I feel that the more important aspect is the shape of the value when it is provided, not as much having the property passed in or not since the developer usually has an idea of which props they want to specify and IDEs still give information on what props are available.

@shadaj shadaj force-pushed the master branch 2 times, most recently from e0c6fd8 to 61e62a7 Compare March 27, 2019 21:22
shadaj added a commit that referenced this pull request Apr 18, 2019
commit 525236746ccf6860643740800de48f3c10a1e6d0
Author: Shadaj Laddad <[email protected]>
Date:   Wed Apr 17 23:50:47 2019 -0700

    Improve facade typings and update changelog

commit c8cd32cc60c95bd01a801007dedbf87e0e2eb1ca
Merge: 9642da1 6db5e37
Author: Shadaj Laddad <[email protected]>
Date:   Wed Apr 17 23:48:16 2019 -0700

    Merge branch 'facades' of git://github.com/dohrayme/slinky into dohrayme-facades

commit 6db5e37
Author: Ray Price <[email protected]>
Date:   Wed Jan 2 16:29:11 2019 +0000

    react-native facades
@shadaj
Copy link
Owner

shadaj commented Apr 18, 2019

Manually merged as 7cbb79b

@shadaj shadaj closed this Apr 18, 2019
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.

2 participants