Skip to content
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

AC-591 Improved UI of login button and FragmentDialogue buttons #672

Merged
merged 1 commit into from
Feb 4, 2020

Conversation

rishabh-997
Copy link
Collaborator

Description of what I changed

I've modified the login_button_color.xml file to adapt it to changes in the background as well.
I've changed the fragment_dialog_layout.xml so that all the custom dialogues now gives a better visual appeal to the app.

Issue I worked on

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

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.

@ribhavsharma
Copy link
Contributor

@rishabh-997 Can you please attach screenshots?

@rishabh-997
Copy link
Collaborator Author

@rishabh-997 Can you please attach screenshots?

yeah, I've posted the images on JIRA issue at link

@codecov-io
Copy link

codecov-io commented Jan 14, 2020

Codecov Report

Merging #672 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #672   +/-   ##
=======================================
  Coverage   12.54%   12.54%           
=======================================
  Files         233      233           
  Lines       10686    10686           
  Branches      993      993           
=======================================
  Hits         1341     1341           
  Misses       9264     9264           
  Partials       81       81

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 61a5350...b590519. Read the comment docs.

@ribhavsharma
Copy link
Contributor

ribhavsharma commented Jan 14, 2020

@rishabh-997 Can you please attach screenshots?

yeah, I've posted the images on JIRA issue at link

Hi rishabh, I checked out your screenshots :)
But According to the Material guidelines, the button text shouldn't be italic. Can we replace these buttons with material buttons?

@f4ww4z
Copy link
Collaborator

f4ww4z commented Jan 17, 2020

But According to the Material guidelines, the button text shouldn't be italic. Can we replace these buttons with material buttons?

@rishabh-997 Did you see this? Also, for darker-colored button backgrounds, the text should be white. E.g. the login button should have white text with that green color background.

Please send the screenshots here in this PR.

@rishabh-997
Copy link
Collaborator Author

But According to the Material guidelines, the button text shouldn't be italic. Can we replace these buttons with material buttons?

@rishabh-997 Did you see this? Also, for darker-colored button backgrounds, the text should be white. E.g. the login button should have white text with that green color background.

Please send the screenshots here in this PR.

was engaged in kotlin migration, will update this today

@rishabh-997
Copy link
Collaborator Author

Done with the changes, here is the screenshot:
image 1
image 2
image 3

@ribhavsharma
Copy link
Contributor

can we use the <MaterialButton> tag instead of the default button? It would not need much styling too. Check this out https://material.io/develop/android/components/material-button/

@rishabh-997
Copy link
Collaborator Author

can we use the <MaterialButton> tag instead of the default button? It would not need much styling too. Check this out https://material.io/develop/android/components/material-button/

I have migrated buttons to Material Buttons but I think that change was not required as I still had to write all the boilerplate code. Nevertheless, Have a look at the PR now.

@ribhavsharma
Copy link
Contributor

@rishabh-997 Please update the screenshots :)

@rishabh-997
Copy link
Collaborator Author

@rishabh-997 Please update the screenshots :)

image

@ribhavsharma
Copy link
Contributor

@rishabh-997 I guess there's an error in your code, because the buttons are not actually visible.

@rishabh-997
Copy link
Collaborator Author

@rishabh-997 I guess there's an error in your code, because the buttons are not actually visible.

Exactly that's why I was not using material button as it overrides many functionalities... I tried using bordered style but it was worthless

@ribhavsharma
Copy link
Contributor

ribhavsharma commented Jan 17, 2020

@rishabh-997 let's see what @f4ww4z says about this, otherwise we won't have any option but to rollback :/

@rishabh-997
Copy link
Collaborator Author

Any suggestions @f4ww4z sir

@ribhavsharma
Copy link
Contributor

@rishabh-997 try playing around with the elevation a bit? also, please keep the same text color for the two buttons to maintain consistency.

@f4ww4z
Copy link
Collaborator

f4ww4z commented Jan 24, 2020

@rishabh-997 Please update the screenshots :)

image

@rishabh-997 how about using the default button, with the background color the same as the appbar's color (dark green)? The text should be white of course. Right now it doesn't look like a button, and it won't be consistent with other buttons in the app.

@rishabh-997 try playing around with the elevation a bit? also, please keep the same text color for the two buttons to maintain consistency.

The default Button should already have elevation I believe. Try using that @rishabh-997 . Login button looks good.

@ribhavsharma
Copy link
Contributor

Please resolve merge conflicts :)

@rishabh-997
Copy link
Collaborator Author

rishabh-997 commented Jan 26, 2020

@f4ww4z sir, I changed the background as per you requested... Please select one of them, I'll update the PR as per that...
try 1

@ribhavsharma
Copy link
Contributor

Looks good Rishabh!

aoolied requested changes

migrated buttons to Material Buttons

r

Improved UI of login button and FragmentDialogue buttons

updated colour of buttons

added colour

Updated UI for buttons
@rishabh-997 rishabh-997 merged commit aa7583a into openmrs:master Feb 4, 2020
tbagz104 pushed a commit to tbagz104/openmrs-contrib-android-client that referenced this pull request Feb 7, 2020
…mrs#672)

I've modified the login_button_color.xml file to adapt it to changes in the background as well.
I've changed the fragment_dialog_layout.xml so that all the custom dialogues now gives a better visual appeal to the app.
@rishabh-997 rishabh-997 deleted the AC-591 branch March 27, 2020 11:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants