-
-
Notifications
You must be signed in to change notification settings - Fork 51
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
Improve UX of crosswalk to webview migration #134
Comments
Relevant conversation for user need and prioritization (internal): |
android support for inline updates documented here |
Added to 4.0.0 because this will be required when we roll the webview version out to Android 5+. We may decide to solve it earlier than that. |
Further discussion with @garethbowen and @mrjones-plip - scheduling in 3.11 as a low priority. If eng time frees up after Angular work is complete, please pick up this ticket. Regardless of whether time is allocated for eng implementation in 3.11, we should aim to have the design and eng solution detailed. In the case that a deployed project running android 10+ receives a de-listing notification from Google, it will become a high-priority. |
Chatting with @n-orlowski , we think there are 3 design components, but not totally sure. We need some additional eng exploration to define design requirements
|
My thoughts...
|
Aligned with @garethbowen on the below:
LMK if I missed anything! |
Nice changes! Does this design supersede this issue scheduled in 3.13? |
@MaxDiz Unfortunately not. That issue is about cht-core upgrades, whereas this one is about migrating data in the android app. |
Based on the roadmap planning discussion, this issue should be added to the next android release to:
|
This should likely be considered blocking for BRAC's android upgrade - they have several thoursand Android 10 users. Play Console has given a deadline of March 29th due to compliance issues. |
@kennsippell @garethbowen @craig-landry , Here are some promising results I got today:
I still need to solve this other issue reported in the ticket:
But I have taken a look to the events triggered when it happens and it shouldn't be hard to fix, though it will involve to implement a new view or two to display the mockups provided by Nicole, so maybe it is best to create a PR to solve first the migration issue, and then another for this last error that also can happens during migrations but is not directly related and less likely to happen. |
Thanks for the update! It looks good to me, but if we can add Nicole's UX to warn about the restart that'll be perfect. I'm happy for this issue to be split into two. The bit you've solved is by far the most important. The service worker cache is unfortunate and may introduce downtime but doesn't risk losing data. |
The pull/162 is ready for review. Changes from my last comments above:
Note that in the recording above the phone uses the dark mode that is why the background is black instead of white as in the mockups. As discussed earlier, we should move forward with the review and testing of this, instead of block the ticket while I start to work next week on:
If the above is easy to solve I would try to push the changes in the same PR next week, otherwise I think because the major issue is solved in the PR it would be better to move forward with a release with what we have. |
Ready for AT, branch The changes don't cover the improvements for the error messages displayed when there are connection errors (moved to #163). What was implemented are the changes mentioned above. Also, here are the steps for a quick test: #162 (comment) |
Tested as follow using a phone with android 9:
Not sure how important this is from a UX perspective @n-orlowski , the spinner is not in same colour as the one in the mock you suggested (looks greenish). Other than that, it looks good to me. Also tested:
|
@ngaruko, I pulled the spinner in the mock from what we are currently using for when the app loads for the first time. We should try to be consistent if possible 🙂 (although it's not a huge UX issue) |
@n-orlowski sorry I didn't clarify about this, the UI that you see is built with an native Android layout, no HTML / CSS. I tried to pull the spinner from the CHT codebase but it is not a animated gif, it is a pure CSS image with CSS animations that I cannot use in the Android layout, so I used a native spinner component that is present in any Android SDK that looks pretty similar. I'm not sure if I can customize the colors of it (that is the big difference), and for sure I cannot customize the border size, but I can give it a try at least to the first. I'll try later, but please @ngaruko lets continue with the "functional" tests in the meantime. |
@n-orlowski I couldn't change the icon, the circle in the spinner is an image that then is rotated (not a gif). Make the icon equals to the CHT one would require create a transparent PNG version for the different configuration sizes, and then replicate the animation with the styles language used in Android. We are short of time so we will continue with the native spinner for now, please @ngaruko continue with the tests with the version there is now. |
@mrsarm I do assume this PR/issue was just about this warning plus checking that the app still functions properly, especially: user not logged out, the upgrade actually happened, no data loss. So, we are good to go, ready to merge. |
Merged into master. As mentioned above, the changes don't cover the improvements for the error messages displayed when there are connection errors, that was moved to #163, and in progress to include both tickets in the next release. |
Right @craig-landry , both improve the migrations process, but are not related, and releasing a new version just with the changes on this ticket is possible because it tackles the major problem when migrating. |
@mrsarm @craig-landry are we waiting for #163 or can we can a release with #134 ? |
Yes @derickl , we are waiting for #163 that is developed but waiting review, if everything goes right, I expected it to be approved today, and maybe tested by QA if there are no important changes to make before releasing an alpha version. Please @garethbowen , could you prioritize the reviewing of pull/165 so we can move forward with a new release this week? |
In 0.6.0 we hastily added a crosswalk to webview data migration and published a webview flavour for Android 10+. While this does the trick it's not the best UX and if we can it'd be good to make it smoother particularly when we decide to roll the webview version to all Android versions.
Firstly, the service worker cache is not migrated, so if the user upgrades and the loses internet an ugly error message is shown.
Secondly, sometimes when the app is first loaded post upgrade the migration hasn't yet finished so the user is taken to the login page. Restarting the app eventually fixes the problem, but we should have some indication that this is the case, and ideally automatically reload when the migration is complete. This is challenging because this migration step is mostly transparent to us, but we could run some JS in the webview checking for the existence of the session cookie which indicates when it's complete.
The text was updated successfully, but these errors were encountered: