-
Notifications
You must be signed in to change notification settings - Fork 119
Find a combination of packages that works reliably #78
base: master
Are you sure you want to change the base?
Conversation
Nice work @isocra , hope they merge this soon, having alot of problems becouse of this... |
When somebody will merge this? |
Just tested the signature example after pulling these changes and it works. |
Yes, please merge! |
@brentvatne : hey Brent, plz take a look and merge it. Bacon look too busy. |
Just to confirm, I've checked this with Expo SDK 33 and it works fine with no changes needed. |
In the meantime, have a look at #77 (comment), this solution works fine with the existing code. |
@EvanBacon recently merged expo/browser-polyfill#19, should that be reflected here? |
@EvanBacon @brentvatne Please merge this PR |
Is this repo abandoned? |
@@ -17,4 +16,4 @@ | |||
}, | |||
"platforms": ["android", "ios", "web"] | |||
} | |||
} | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a newline at the end of the file.
@@ -0,0 +1,7269 @@ | |||
Arguments: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this file committed?
this.setState({ appState: nextAppState }); | ||
}; | ||
|
||
componentDidMount() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe removal of this will break switching apps in Android - the drawing will be cleared upon return to the app.
import { PanResponder, PixelRatio } from 'react-native'; | ||
import { vec2 } from 'gl-matrix'; | ||
import { | ||
GLView |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you end split collections with a trailing comma, when someone else comes to add to the end of the collection in future, they won't need to change the last line, and overwrite the git blame
for it, and making the PR harder to read.
import { PanResponder, PixelRatio } from 'react-native'; | ||
import { vec2 } from 'gl-matrix'; | ||
import { | ||
GLView |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What benefit is there to splitting collections that fit very comfortably on a single line?
.idea/* | ||
.history/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your site-specific clutter should not be added to the repo-wide .gitignore
. Use your local git config, or the private .git/info/exclude
.
/> | ||
); | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Newline at the end of files please.
} | ||
ref = { | ||
this.setRef | ||
} { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a terrible place to break a line - ref={this.setRef}
and {...this.props}
are two entirely different props. Technically {...this.props}
is a set of zero of more props, but it's still not ref
.
This entire block hasn't changed - why are you messing with the layout of it? All that's doing is making it harder to read for the person who wrote it in the first place.
@@ -99,4 +99,4 @@ | |||
"eslintConfig": { | |||
"extends": "expo" | |||
} | |||
} | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Newline at the end of files, please.
@@ -1,10786 +0,0 @@ | |||
# THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Asday thanks for all the comments on the PR. My aim was to make the minimum changes so that I can add Signatures to my Expo app, rather than fix everything. However, if you'd like to fix these in my fork, that would be great. |
As per issue #77 here is a combination of packages that seem to work reliably using
yarn
(tested by removingnode_modules
andyarn.lock
and then reinstalling everything. I tried"@expo/browser-polyfill": "0.0.1-alpha.4"
but that didn't work so left it atalpha.3
.