Desktop two-pane UI with react-navigation v3 #8630
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Pull Request Checklist
|
Jenkins BuildsClick to see older builds (204)
|
vkjr
force-pushed
the
feature/2_pane_ui_on_desktop
branch
from
49ab714
to
de94ae4
Compare
|
@annadanchenko, desktop builds in this PR has mobile 2-pane UI working. Can we make a full testing to find all issues that need to be merged before fixing?
|
|
Took the UI for a testrun. Device
Some quick observations:
IMO this UI is more suitable on a tablet device. Will give it try on 10.1" touch 1920 x 1200 next. |
|
@hesterbruikman, thanks for checking! yes, this UI now is purely mobile, without customization for desktop. I'd be glad if we can merge it in develop after solving technical issues (like Text input in chat) and leave UI notes to be fixed later in a separate PRs. Wdyt, would it be ok? |
Sure! Let me know where to best capture issues for future PR's. |
|
I think they can be created right here, on github. Maybe new label would be convenient, something like |
9 tasks
Thanks @vkjr! See here a list by myself and @errorists. Please check in if anything needs clarification! |
|
Thanks! |
Labels can be renamed here: |
|
@vkjr well done, love it so far on mac! :) for some reason it works faster and UI more responsive |
|
@flexsurfer r u sure? Weird, myself and some other folks have the exact opposite experience, that it’s more laggy. Maybe there’s some variable in play like Mac OS version or config, dunno. |
|
switching between chats and scrolling messages much faster, dunno, maybe you need pro mac for that :D |
|
yes after some time it gets slower and slower and more buggy :( |
|
@annadanchenko Yes, I have, but it is still WIP - so not sure what exactly should be fixed from #7551 and what areas should work for now (i.e. chats only or chats + wallet + browser) and what platforms I can test. |
vkjr
force-pushed
the
feature/2_pane_ui_on_desktop
branch
from
0da5722
to
e242b4d
Compare
|
@churik, @annadanchenko, I don't know what was a problem with building last commits, I think it is something with a build system, not the code. |
vkjr
force-pushed
the
feature/2_pane_ui_on_desktop
branch
from
e242b4d
to
c89b9ae
Compare
vkjr
force-pushed
the
feature/2_pane_ui_on_desktop
branch
from
8950ccd
to
e1e91f8
Compare
|
@yenda, @pombeirp, I synced |
vkjr
dismissed
yenda’s stale review
Comments are fixed but @yenda is on vacation
85% of end-end tests have passedFailed tests (7)Click to expand
Passed tests (40)Click to expand |
89% of end-end tests have passedFailed tests (5)Click to expand
Passed tests (42)Click to expand |
|
@flexsurfer, @churik, @pombeirp, all failed e2e tests are the same as in other merged pr. I suggest to merge this PR, wdyt? |
flexsurfer
reviewed
Oct 2, 2019
| @@ -83,6 +83,11 @@ | |||
| {:height tabs-height | |||
| :align-self :stretch | |||
| :ios {:background-color :white | |||
| :shadow-radius 4 | |||
There was a problem hiding this comment.
There is no change in ios style here, I just added a copy of this style for desktop (if I correctly understood your question). Changes highlighted incorrectly in github.
flexsurfer
reviewed
Oct 2, 2019
| @@ -89,7 +89,6 @@ | |||
| :auto-capitalize :sentences} | |||
| (when cooldown-enabled? | |||
| {:placeholder (i18n/label :cooldown/text-input-disabled)}))])) | |||
| ;) | |||
There was a problem hiding this comment.
I removed commented out brace because it is leftover and it shouldn't be there.
flexsurfer
approved these changes
Oct 2, 2019
Signed-off-by: Volodymyr Kozieiev <vkjr.sp@gmail.com>
vkjr
force-pushed
the
feature/2_pane_ui_on_desktop
branch
from
e1e91f8
to
ace4b5a
Compare
|
@vkjr |
|
@churik, yes, I used it locally for chatting |
|
Merged this after discussion with @flexsurfer. All fixes will be made in a separate PRs. |
|
For now we should describe briefly what was tested and on what platforms (after changing QA process). |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fixes #7399
Summary
This PR contains desktop mobile UI updated to work with react-navigation v3
Review notes
Testing notes
Suggestion is to test only mobile platforms.
Platforms
Areas that maybe impacted
Functional
Non-functional
Steps to test
status: wip