Skip to content

[react-native] Support Layout Props#25083

Merged
weswigham merged 1 commit intoDefinitelyTyped:masterfrom
brunolemos:patch-11
Apr 25, 2018
Merged

[react-native] Support Layout Props#25083
weswigham merged 1 commit intoDefinitelyTyped:masterfrom
brunolemos:patch-11

Conversation

@brunolemos
Copy link
Copy Markdown
Contributor

@brunolemos brunolemos commented Apr 18, 2018 •
edited
Loading

Related: #25081

TIL React Native supports layout styles as View and Image props.
E.g.: <View flex={1} />, <Image width={200} />

  • Use a meaningful title for the pull request. Include the name of the package modified.
  • Test the change in your own code. (Compile and run.)
  • Add or edit tests to reflect the change. (Run with npm test.)
  • Follow the advice from the readme.
  • Avoid common mistakes.
  • Run npm run lint package-name (or tsc if no tslint.json is present).
  • Provide a URL to documentation or source code which provides context for the suggested changes: Docs | Components search
  • Increase the version number in the header if appropriate.
  • If you are making substantial changes, consider adding a tslint.json containing { "extends": "dtslint/dt.json" }.

@brunolemos brunolemos requested a review from alloy as a code owner April 18, 2018 06:24
@brunolemos brunolemos changed the title Support Layout Props [react-native] Support Layout Props Apr 18, 2018
@brunolemos brunolemos force-pushed the patch-11 branch 2 times, most recently from 5d9005a to 232793c Compare April 18, 2018 06:31
@brunolemos
Copy link
Copy Markdown
Contributor Author

brunolemos commented Apr 18, 2018 •
edited
Loading

Hm, it's conflicting with react-native-modalbox because they are overriding the position prop. What to do?

BTW seems to be a deprecated lib, last update 9 months ago.

@typescript-bot typescript-bot added Popular package This PR affects a popular package (as counted by NPM download counts). The Travis CI build failed labels Apr 18, 2018
@typescript-bot
Copy link
Copy Markdown
Contributor

typescript-bot commented Apr 18, 2018 •
edited
Loading

@brunolemos Thank you for submitting this PR!

🔔 @alloy @huhuanming @iRoachie @timwangdev @kamal @nelyousfi @alexdunne @swissmanu @bm-software - please review this PR in the next few days. Be sure to explicitly select Approve or Request Changes in the GitHub UI so I know what's going on.

If no reviewer appears after a week, a DefinitelyTyped maintainer will review the PR instead.

@typescript-bot
Copy link
Copy Markdown
Contributor

typescript-bot commented Apr 18, 2018 •
edited
Loading

@brunolemos The Travis CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

@iRoachie
Copy link
Copy Markdown
Contributor

iRoachie commented Apr 19, 2018

Doesn't actually seem to be the case of inheriting from ViewProperties, https://github.com/maxs15/react-native-modalbox/blob/master/index.js. May have been a error on my part when doing the original typings. So I guess we can change the typing of react-native-modalbox.

@iRoachie
Copy link
Copy Markdown
Contributor

iRoachie commented Apr 19, 2018

Only prop they seem to use is style https://github.com/maxs15/react-native-modalbox/blob/master/index.js#L426. I can patch this in

@iRoachie iRoachie mentioned this pull request Apr 19, 2018
Merged
7 tasks
@brunolemos
Copy link
Copy Markdown
Contributor Author

brunolemos commented Apr 19, 2018

Seems to conflict with lot's of packages :(

@typescript-bot
Copy link
Copy Markdown
Contributor

typescript-bot commented Apr 19, 2018 •
edited
Loading

@brunolemos The Travis CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

@iRoachie
Copy link
Copy Markdown
Contributor

iRoachie commented Apr 19, 2018

Just react-native-linear-gradient and expo

@brunolemos
Copy link
Copy Markdown
Contributor Author

brunolemos commented Apr 19, 2018

@iRoachie but is it showing all errors? I'm afraid it's only showing for the end property now, like previously it only showed for the position property. Let me try locally.

@iRoachie
Copy link
Copy Markdown
Contributor

iRoachie commented Apr 19, 2018

May have to run again, since the modalbox PR just got merged

@brunolemos
Copy link
Copy Markdown
Contributor Author

brunolemos commented Apr 19, 2018

Yes I pushed again a few minutes ago

@brunolemos
Copy link
Copy Markdown
Contributor Author

brunolemos commented Apr 19, 2018 •
edited
Loading

@iRoachie I've checked all properties, so these are all pending errors:

Incompatible properties with packages

  • position
    • react-native-modalbox
  • end, start
    • react-native-linear-gradient
      • expo

Other compiler errors

  • react-native-google-signin

Possibily not related to this PR?

react-native-google-signin failing:
Error: /Users/brunolemos/Projects/packages/DefinitelyTyped/types/react-native-google-signin/react-native-google-signin-tests.tsx:18:32
ERROR: 18:32  expect  TypeScript@next compile error: 
Type 'PromiseConstructor' is not a valid async function return type in ES5/ES3 because it does not refer to a Promise-compatible constructor value.
ERROR: 18:32  expect  TypeScript@next compile error: 
Type 'PromiseConstructor' is not a valid async function return type in ES5/ES3 because it does not refer to a Promise-compatible constructor value.
  Types of parameters 'executor' and 'executor' are incompatible.
    Types of parameters 'resolve' and 'resolve' are incompatible.
      Type '[T] extends [void] ? (value?: T | PromiseLike<T> | undefined) => void : (value: T | PromiseLike<T...' is not assignable to type '(value?: T | PromiseLike<T> | undefined) => void'.
        Type '((value?: T | PromiseLike<T> | undefined) => void) | ((value: T | PromiseLike<T>) => void)' is not assignable to type '(value?: T | PromiseLike<T> | undefined) => void'.
          Type '(value: T | PromiseLike<T>) => void' is not assignable to type '(value?: T | PromiseLike<T> | undefined) => void'.
            Types of parameters 'value' and 'value' are incompatible.
              Type 'T | PromiseLike<T> | undefined' is not assignable to type 'T | PromiseLike<T>'.
                Type 'undefined' is not assignable to type 'T | PromiseLike<T>'.

Error in react-native-google-signin
Error: /Users/brunolemos/Projects/packages/DefinitelyTyped/types/react-native-google-signin/react-native-google-signin-tests.tsx:18:32
ERROR: 18:32  expect  TypeScript@next compile error: 
Type 'PromiseConstructor' is not a valid async function return type in ES5/ES3 because it does not refer to a Promise-compatible constructor value.
ERROR: 18:32  expect  TypeScript@next compile error: 
Type 'PromiseConstructor' is not a valid async function return type in ES5/ES3 because it does not refer to a Promise-compatible constructor value.
  Types of parameters 'executor' and 'executor' are incompatible.
    Types of parameters 'resolve' and 'resolve' are incompatible.
      Type '[T] extends [void] ? (value?: T | PromiseLike<T> | undefined) => void : (value: T | PromiseLike<T...' is not assignable to type '(value?: T | PromiseLike<T> | undefined) => void'.
        Type '((value?: T | PromiseLike<T> | undefined) => void) | ((value: T | PromiseLike<T>) => void)' is not assignable to type '(value?: T | PromiseLike<T> | undefined) => void'.
          Type '(value: T | PromiseLike<T>) => void' is not assignable to type '(value?: T | PromiseLike<T> | undefined) => void'.
            Types of parameters 'value' and 'value' are incompatible.
              Type 'T | PromiseLike<T> | undefined' is not assignable to type 'T | PromiseLike<T>'.
                Type 'undefined' is not assignable to type 'T | PromiseLike<T>'.

@iRoachie
Copy link
Copy Markdown
Contributor

iRoachie commented Apr 19, 2018

How do we go about fixing these? Changing prop names in libraries would have to be breaking changes for their packages.

@brunolemos
Copy link
Copy Markdown
Contributor Author

brunolemos commented Apr 19, 2018 •
edited
Loading

@iRoachie

Solution 1: I can make a PR to react-native-linear-gradient and expo, but the lastest release from react-native-linear-gradient is from 5 months ago. I doubt they would make a new release (and with a breaking change). (edit: @sibelius may be able to help with a new release).
PR: react-native-linear-gradient/react-native-linear-gradient#280

Solution 2: We can comment these two properties here for now and merge this, to prevent other properties from conflicting in the future. And create an issue. The problem with this approach is that they will probably end up commented forever.

Solution 3: We can remove react-native-linear-gradient typings from this repo as they have a index.d.ts in their repo, but we also need to remove the LinearGradientProps and LinearGradient from the expo typings.

@brunolemos brunolemos mentioned this pull request Apr 19, 2018
Closed
brunolemos added a commit to brunolemos/react-native-linear-gradient that referenced this pull request Apr 19, 2018
@brunolemos
Rename start and end props to startPoint and endPoint
d9e3c8a 
Fix react-native-linear-gradient#279

Fix conflict with react native layout props

Related: DefinitelyTyped/DefinitelyTyped#25083
@brunolemos brunolemos mentioned this pull request Apr 19, 2018
Closed
@iRoachie
Copy link
Copy Markdown
Contributor

iRoachie commented Apr 19, 2018

Alright number 3 sounds good to me

brunolemos added a commit to brunolemos/DefinitelyTyped that referenced this pull request Apr 19, 2018
@brunolemos
[expo] Comment LinearGradient typings to unblock DefinitelyTyped#25083
225759d
brunolemos added a commit to brunolemos/DefinitelyTyped that referenced this pull request Apr 19, 2018
@brunolemos
[expo] Comment LinearGradient typings to unblock DefinitelyTyped#25083
002d51e
@brunolemos brunolemos mentioned this pull request Apr 19, 2018
Closed
9 tasks
@brunolemos
Copy link
Copy Markdown
Contributor Author

brunolemos commented Apr 19, 2018 •
edited
Loading

@iRoachie ok, made these PRs:

brunolemos added a commit to brunolemos/DefinitelyTyped that referenced this pull request Apr 19, 2018
@brunolemos
[expo] Un-extend from ViewProperties to unblock DefinitelyTyped#25083
0e07832
@brunolemos brunolemos mentioned this pull request Apr 19, 2018
Merged
9 tasks
brunolemos added a commit to brunolemos/DefinitelyTyped that referenced this pull request Apr 19, 2018
@brunolemos
[expo] Un-extend LinearGradientProps from ViewProperties to unblock D…
2c91e48 
…efinitelyTyped#25083
weswigham pushed a commit that referenced this pull request Apr 25, 2018
@brunolemos @weswigham
[expo] Un-extend LinearGradientProps from ViewProperties to unblock #…
a5cb711 
…25083 (#25149)
@brunolemos
[react-native] Support Layout Props
eaedb21 
Support some styles as View and Image props, e.g. <View flex={1} />
@brunolemos
Copy link
Copy Markdown
Contributor Author

brunolemos commented Apr 25, 2018

@iRoachie ok, ready 🍾

iRoachie
iRoachie approved these changes Apr 25, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@iRoachie iRoachie left a comment

Choose a reason for hiding this comment

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

Thanks for taking the time to do this

@weswigham weswigham merged commit fdb2c25 into DefinitelyTyped:master Apr 25, 2018
@brunolemos brunolemos mentioned this pull request May 12, 2018
Merged
9 tasks
@brunolemos brunolemos mentioned this pull request Aug 18, 2018
Merged
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alloy alloy Awaiting requested review from alloy

1 more reviewer

@iRoachie iRoachie iRoachie approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Popular package This PR affects a popular package (as counted by NPM download counts).

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@brunolemos @typescript-bot @iRoachie @weswigham