AC-665 Updated UI for Login Page by rishabh-997 · Pull Request #743 · openmrs/openmrs-contrib-android-client

rishabh-997 · 2020-05-18T10:48:17Z

Description of what I changed

Created a better UI design
Used constraint layout to flatten the hierarchy as much as possible
Used View Binding instead of binding views explicitly so that changing view type won't affect code.
Refactored code to reduce ~100 lines of code in LoginFragment

Issue I worked on

JIRA Issue: https://issues.openmrs.org/browse/AC-665

Checklist: I completed these to help reviewers :)

My pull request only contains ONE single commit
(the number above, next to the 'Commits' tab is 1).

I have added tests to cover my changes. (If you refactored
existing code that was well tested you do not have to add tests)

All new and existing tests passed.

My pull request is based on the latest changes of the master branch.

rishabh-997 · 2020-05-18T10:51:49Z

codecov-io · 2020-05-18T11:00:09Z

Codecov Report

Merging #743 into master will increase coverage by 0.03%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master     #743      +/-   ##
==========================================
+ Coverage   11.76%   11.79%   +0.03%     
==========================================
  Files         238      238              
  Lines       10646    10602      -44     
  Branches     1027     1025       -2     
==========================================
- Hits         1252     1251       -1     
+ Misses       9312     9269      -43     
  Partials       82       82

Impacted Files	Coverage Δ
...openmrs/mobile/activities/login/LoginFragment.java	`5.49% <0.00%> (+1.22%)`	⬆️
...penmrs/mobile/activities/login/LoginPresenter.java	`65.49% <ø> (-0.25%)`	⬇️
.../org/openmrs/mobile/activities/ACBaseActivity.java	`0.00% <0.00%> (ø)`
...openmrs/mobile/utilities/ApplicationConstants.java	`5.26% <0.00%> (ø)`
...s/mobile/activities/settings/SettingsActivity.java	`0.00% <0.00%> (ø)`
...s/mobile/activities/settings/SettingsFragment.java	`0.00% <0.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3778fdf...fd53abb. Read the comment docs.

f4ww4z

@rishabh-997 Can you also vertically center the 'Get started with our user guide' text?

f4ww4z · 2020-05-19T15:23:27Z

openmrs-client/src/main/java/org/openmrs/mobile/activities/login/LoginPresenter.java

                        loginView.setLocationErrorOccurred(false);
                    } else {
-                        loginView.showInvalidURLSnackbar("Failed to fetch server's locations");
+                        loginView.showInvalidURLSnackbar("The entered server is currently down, please select a different one.");


Put this string as a string resource instead

Use snackbar_server_error

Done,

Had to pass context from activity to extract string resource from not Activity class.

Had to update tests accordingly.

Uploaded screenshots with the Get Started aligned both ways. It is currently aligned to left in the PR, so if you like the central way, I'll update the PR

The test fails due to some reason which I can't figure out why, is it some error from my side ???

rishabh-997 · 2020-05-19T15:27:30Z

I tried it and it wasn't looking appealing. I can add another screenshot here with that text vertically alligned in middle

codecov-commenter · 2020-05-19T17:59:44Z

Codecov Report

Merging #743 into master will increase coverage by 0.04%.
The diff coverage is 12.24%.

@@            Coverage Diff             @@
##           master     #743      +/-   ##
==========================================
+ Coverage   11.80%   11.85%   +0.04%     
==========================================
  Files         238      238              
  Lines       10650    10598      -52     
  Branches     1028     1025       -3     
==========================================
- Hits         1257     1256       -1     
+ Misses       9311     9260      -51     
  Partials       82       82

Impacted Files	Coverage Δ
...openmrs/mobile/activities/login/LoginActivity.java	`0.00% <0.00%> (ø)`
...openmrs/mobile/activities/login/LoginFragment.java	`5.26% <0.00%> (+1.11%)`	⬆️
...penmrs/mobile/activities/login/LoginPresenter.java	`65.49% <63.15%> (-0.25%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 80c4367...9fc4f1c. Read the comment docs.

VibhorChinda · 2020-05-19T18:08:00Z

I tried it and it wasn't looking appealing. I can add another screenshot here with that text vertically alligned in middle

@rishabh-997 if you think that it does not look appealing. Try centre aligning the text and move it closer to the bottom rather than keeping it closer to the switch. Maybe it will look better :)

rishabh-997 · 2020-05-19T18:15:39Z

I tried it and it wasn't looking appealing. I can add another screenshot here with that text vertically alligned in middle

@rishabh-997 if you think that it does not look appealing. Try centre aligning the text and move it closer to the bottom rather than keeping it closer to the switch. Maybe it will look better :)

I have added screenshot as you said, have a look at all three of them. Let me know your suggestions

f4ww4z · 2020-05-21T15:24:49Z

@rishabh-997 It looks more appealing with the get started at the center. The first screenshot looks good. Also make the 'Unable to Login' text look more like a link / button - I suggest to make it an orange color. Please change the code to reflect this.

rishabh-997 · 2020-05-21T17:13:29Z

dino-saurabh

@rishabh-997 I would like to suggest a thing like to avoid confusion for users can we give them options for alternate refApp server along with the demo server so that users dont have go on looking for alternate servers URl.

rishabh-997 · 2020-05-22T20:02:39Z

@rishabh-997 I would like to suggest a thing like to avoid confusion for users can we give them options for alternate refApp server along with the demo server so that users dont have go on looking for alternate servers URl.

We have now updated the snack bar so when the server is down, there is an apt message regarding server down error along with a link with all the alternate servers... Are you suggesting anything different ????

dino-saurabh · 2020-05-23T06:53:25Z

@rishabh-997 I thought the users getting redirected to some external link will be a bad UX maybe have some sort of drop down to select from a list of servers thats what I thought !!

f4ww4z · 2020-06-01T03:12:50Z

@rishabh-997 Can you fix the merge conflicts and refactor according to the new localization changes ? Then we can merge.

f4ww4z

@rishabh-997 Make sure to eliminate these conflicts and that you can build the app locally.

f4ww4z · 2020-06-02T03:19:07Z

openmrs-client/src/test/java/org/openmrs/mobile/test/presenters/LoginPresenterTest.java

+<<<<<<< HEAD
+=======
+
+>>>>>>> removed context from presenter


@rishabh-997 these lines should be removed

f4ww4z · 2020-06-02T03:19:27Z

openmrs-client/src/test/java/org/openmrs/mobile/test/presenters/LoginPresenterTest.java

+<<<<<<< HEAD
            openMRSLogger, authorizationManager);
+=======
+                openMRSLogger, authorizationManager);
+>>>>>>> removed context from presenter


Take care of this merge conflict

f4ww4z · 2020-06-02T03:19:39Z

openmrs-client/src/test/java/org/openmrs/mobile/test/presenters/LoginPresenterTest.java

+<<<<<<< HEAD
            .thenReturn(mockErrorCall(401));
+=======
+                .thenReturn(mockErrorCall(401));
+>>>>>>> removed context from presenter


@rishabh-997 see above comment

f4ww4z · 2020-06-02T03:19:57Z

openmrs-client/src/test/java/org/openmrs/mobile/test/presenters/LoginPresenterTest.java

+<<<<<<< HEAD
        verify(view).showInvalidURLSnackbar(R.string.failed_fetching_servers_location);
+=======
+        verify(view).showInvalidURLSnackbar(R.string.snackbar_server_error);
+>>>>>>> removed context from presenter


@rishabh-997 See above comment

f4ww4z

Good work @rishabh-997

rishabh-997 changed the title ~~AC-995 Updated UI for Login Page~~ AC-665 Updated UI for Login Page May 18, 2020

f4ww4z requested changes May 19, 2020

View reviewed changes

f4ww4z requested a review from VibhorChinda May 19, 2020 15:26

rishabh-997 force-pushed the AC-665-loginUI branch from e277bea to dc98d1f Compare May 19, 2020 17:55

rishabh-997 force-pushed the AC-665-loginUI branch from dc98d1f to b12decc Compare May 19, 2020 18:56

dino-saurabh requested changes May 22, 2020

View reviewed changes

rishabh-997 force-pushed the AC-665-loginUI branch from c9223bf to a231452 Compare May 26, 2020 19:45

dino-saurabh approved these changes May 30, 2020

View reviewed changes

rishabh-997 force-pushed the AC-665-loginUI branch from f0284e4 to 33f20d2 Compare June 1, 2020 06:27

f4ww4z requested changes Jun 2, 2020

View reviewed changes

rishabh-997 added 4 commits June 2, 2020 11:18

Resolving conflicts

e166a8a

Updated Login page UI as requested

a88408d

resolved conflicts

7cddc1e

Resolving conflicts

75b0f08

rishabh-997 force-pushed the AC-665-loginUI branch from 33f20d2 to 75b0f08 Compare June 2, 2020 05:49

resolved conflicts on LoginTest

9fc4f1c

f4ww4z approved these changes Jun 6, 2020

View reviewed changes

f4ww4z merged commit 547cc1e into openmrs:master Jun 6, 2020

rishabh-997 deleted the AC-665-loginUI branch June 6, 2020 04:04

Conversation

rishabh-997 commented May 18, 2020

Description of what I changed

Issue I worked on

Checklist: I completed these to help reviewers :)

Uh oh!

rishabh-997 commented May 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented May 18, 2020

Codecov Report

Uh oh!

f4ww4z left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rishabh-997 commented May 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented May 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

VibhorChinda commented May 19, 2020

Uh oh!

rishabh-997 commented May 19, 2020

Uh oh!

f4ww4z commented May 21, 2020

Uh oh!

rishabh-997 commented May 21, 2020

Uh oh!

dino-saurabh left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rishabh-997 commented May 22, 2020

Uh oh!

dino-saurabh commented May 23, 2020

Uh oh!

f4ww4z commented Jun 1, 2020

Uh oh!

f4ww4z left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

f4ww4z left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

rishabh-997 commented May 18, 2020 •

edited

Loading

rishabh-997 commented May 19, 2020 •

edited

Loading

codecov-commenter commented May 19, 2020 •

edited

Loading

dino-saurabh left a comment •

edited

Loading