-
Notifications
You must be signed in to change notification settings - Fork 788
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
fix(new-issue): various submit new issue improvement #243
fix(new-issue): various submit new issue improvement #243
Conversation
Look #207 please, I did there for Android, but I need to look at iOS. UPD: Can there be a fix if there is a PR for this? |
routes.js
Outdated
@@ -135,6 +135,7 @@ const sharedRoutes = { | |||
|
|||
return { | |||
title: isPR ? `Pull Request #${number}` : `Issue #${number}`, | |||
headerLeft: navigation.state.params.headerLeft, |
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.
Although this would essentially solve hiding the header on the left, have you tried swiping the screen from the left side? That might still allow the user to navigate back which we wouldn't want.
Also see comment 2 about possibly simplifying this flow 🕺
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.
You're right, I haven't tried to swipe left, and it still allow the user to navigate back...
(-> Comment 2)
const { issueTitle, issueComment } = this.state; | ||
|
||
return ( | ||
<ViewContainer> | ||
{isPendingSubmitting && <LoadingModal />} |
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.
<3
@@ -90,17 +92,19 @@ class NewIssue extends Component { | |||
).then(issue => { | |||
navigation.navigate('Issue', { | |||
issue, | |||
headerLeft: false, |
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.
Comment #2
So instead of passing a prop to hide the header, it might just be simpler to use navigation.reset
to reset
the current navigation stack instead of pushing another route on top. That way there shouldn't be a navigate back functionality at all and we wouldn't have to workaround trying to pass a manual prop to hide it.
Let me know what you think @Antoine38660 :)
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 tried to use the reset function (defined in utils/method-helpers.js
) but I had an error, something like:
'Issue' key is not defined, you should use Login, Main ...
I'll try now with navigation.reset
directly 😃
Thanks for reviewed this!
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.
With navigation.reset
, we can't navigate back ✅
BUT the bottom menu item selected when we create the issue can no longer be used: It redirect back to the created issue instead of its normal behavior... 😞
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 that error message is most likely because its only on the top Stack (see here) hence why it can't recognize the Issue
screen 🤔
Glad to hear that just using .reset
worked (somewhat) :P. The bottom menu item? Are you referring to the tab navigator?
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.
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 snap, didn't think of that 🤔
So double tapping the a single tab navigator will take you back to the root
of that stack because of this code here. However, using navigation.reset
in this step will cause that to be the root of the stack which we don't really want.
This is a little tricker than I thought it would be 😂. There's an attribute that we should easily be able to extend to the issue screen to prevent swipe backs (react-navigation/react-navigation#1063 (comment)) but I don't want to always disable it for the screen.
Some thoughts:
- Maybe we can use this attribute but connect to navigation params so that it's only false based on the param (which we can set only on this navigate step to
Issue Screen
? That way with this +headerLeft: null
would not allow the user to go back. Sounds easier said than done 🤔 - Maybe we can leverage a
modal
screen here instead? Haven't tried to load a modal with React Navigation yet but maybe it might be worthwhile to try here?
Either way, happy to merge this in if you would rather we deal with this as part of a separate bug ticket 🎉
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 can try to implement the 1) (with gesturesEnabled: false
+ headerLeft: null
) 😄
But I don't understand the second option with modal :(
@@ -140,7 +144,6 @@ class NewIssue extends Component { | |||
<TextInput | |||
underlineColorAndroid={'transparent'} | |||
placeholder={translate('issue.newIssue.writeAComment', language)} | |||
blurOnSubmit |
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.
🎉
cdc0f68
to
33efa6d
Compare
33efa6d
to
89fd08f
Compare
It works like a charm mate 😃 I disabled swipe back with |
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.
Yaaas thank you so much my friend @Antoine38660 🙌
Also give ability to add new line in the issue body