-
Notifications
You must be signed in to change notification settings - Fork 0
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
Updated React Native example #4
Conversation
ac364cd
to
5e12c14
Compare
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.
Looks good, I dont have much to say about the react implementation.
example/App.js
Outdated
<RootStack.Screen name="Sign In" component={SignInScreen} | ||
options={({ navigation }) => ({ | ||
headerShadowVisible: false, | ||
headerTitleStyle: { fontWeight: 'bold' }, |
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.
FYI iOS navigation titles are semi bold if you want to match it exactly.
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.
ah ok, looks like that is 600
in RN terms, no semibold
out of box https://gist.github.com/knowbody/c5cdf26073b874eae86ba96e7cf3a540
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.
Tangential, but I wonder if it's better if we use those values in the experience model for fontWeight
instead of the named ones. The names ones might be pretty iOS-specific?
@andretortolano any preference?
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 could add more comments about various lines and things, but I don't think it's all that important for this example to look like our in-house style 🤷
const [userID, setUserID] = useState('default-00000'); | ||
|
||
// Initialize the Appcues SDK as early as possible in your app | ||
Appcues.setup('ACCOUNT_ID', 'APP_ID') |
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 should probably be inside a useEffect
so that it only runs once; this component can re-render if setUserID
is called.
useEffect(() => {
Appcues.setup('ACCOUNT_ID', 'APP_ID');
}, []);
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.
ah got it, will do thanks
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.
turns out this does cause some additional complication, as the useEffect
hook only runs after the first render, and there is some code during that render that attempts to access/use the SDK for a screen view during a useFocusEffect
hook. Not sure what best practice there would be yet.
There aren't really that many updates here, but I think the package-lock.json makes it look that way.
key updates:
reset()
,anonymous()
andgroup(id, props)
The example app contents is probably the most interesting thing here. The structure of that example is now: