-
Notifications
You must be signed in to change notification settings - Fork 1.4k
[IMPROVE] Onboarding changes #3387
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
Conversation
| try { | ||
| await Linking.openURL('https://cloud.rocket.chat/trial'); | ||
| } catch { | ||
| logEvent(events.RL_CREATE_NEW_WORKSPACE_F); |
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 there's an issue happening, log the error of the issue.
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.
Just remove it and keep log(e) only.
logEvent isn't important here, although you might seen it on some other catches (they are also wrong).
The only important tracking here is the error itself, so we can fix it later.
| constructor(props) { | ||
| super(props); | ||
| if (!isTablet) { | ||
| Orientation.lockToPortrait(); |
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.
We need to keep locking orientation
| alignSelf: 'center', | ||
| marginTop: isTablet ? 0 : verticalScale(116), | ||
| marginBottom: verticalScale(50), | ||
| maxHeight: verticalScale(150), |
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 we stick to scaling, remember to do it on render instead.
There was an issue when the app was started on landscape and styles calculated on styles.js.
| try { | ||
| await Linking.openURL('https://cloud.rocket.chat/trial'); | ||
| } catch { | ||
| logEvent(events.RL_CREATE_NEW_WORKSPACE_F); |
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.
Just remove it and keep log(e) only.
logEvent isn't important here, although you might seen it on some other catches (they are also wrong).
The only important tracking here is the error itself, so we can fix it later.
…hat.ReactNative into new.create-workspace
|
This pull request introduces 1 alert when merging 996b324 into 0871849 - view on LGTM.com new alerts:
|
|
This pull request introduces 2 alerts when merging db30859 into 0871849 - view on LGTM.com new alerts:
|
|
This pull request introduces 2 alerts when merging 0e43cb0 into 0871849 - view on LGTM.com new alerts:
|
|
This pull request introduces 2 alerts when merging 7dfa547 into a52199a - view on LGTM.com new alerts:
|
…hat.ReactNative into new.create-workspace
diegolmello
left a comment
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.
LGTM! 🚀
- Change the first screen of the app - Minor changes on NewServerView and make it the first screen of the app - Add "Create workspace" to ServerDropdown Co-authored-by: Diego Mello <[email protected]>


Proposed changes
-- This requires a few changes on the stack as well
Issue(s)
How to test or reproduce
Add a server, you must be redirected to the first pageScreenshots
Types of changes
Checklist
Further comments