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.

Sign up for GitHub

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

Jump to bottom

AC-625: Check if newly created provider is similar to an existing one #613

Merged
merged 1 commit into from
Jul 11, 2019
Merged

AC-625: Check if newly created provider is similar to an existing one #613

merged 1 commit into from
Jul 11, 2019

Conversation

deepak140596
Copy link
Contributor

@deepak140596 deepak140596 commented Jul 6, 2019 •
edited
Loading

Description of what I changed

  • Added a check for matching providers
  • Created an AlertDialog to show matching providers

Issue I worked on

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

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.

@codecov-io
Copy link

codecov-io commented Jul 6, 2019 •
edited
Loading

Codecov Report

Merging #613 into master will decrease coverage by <.01%.
The diff coverage is 12.5%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #613      +/-   ##
=========================================
- Coverage   14.31%   14.3%   -0.01%     
=========================================
  Files         194     195       +1     
  Lines        9028    9093      +65     
  Branches      785     790       +5     
=========================================
+ Hits         1292    1301       +9     
- Misses       7657    7713      +56     
  Partials       79      79
Impacted Files Coverage Δ
...openmrs/mobile/utilities/ApplicationConstants.java 0% <ø> (ø) ⬆️
...agerdashboard/addprovider/AddProviderContract.java 0% <ø> (ø) ⬆️
...gerdashboard/ProviderManagerDashboardFragment.java 0% <0%> (ø) ⬆️
...dprovider/MatchingProviderRecyclerViewAdapter.java 0% <0%> (ø)
...agerdashboard/addprovider/AddProviderFragment.java 0% <0%> (ø) ⬆️
...d/ProviderManagerDashboardRecyclerViewAdapter.java 0% <0%> (ø) ⬆️
...gerdashboard/addprovider/AddProviderPresenter.java 96.66% <100%> (+1.42%) ⬆️

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 ed4619f...dcef39b. Read the comment docs.

@deepak140596 deepak140596 changed the title AC-625: Check matching Providers AC-625: Check if newly created provider is similar to an existing one Jul 6, 2019
f4ww4z
f4ww4z requested changes Jul 8, 2019
View reviewed changes
Copy link
Collaborator

@f4ww4z f4ww4z left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

similar providers draft 1

Thanks @deepak140596 . Can you add a box for each provider row item in this dialog? Use a background color from color.xml res.

Also, address my other comments.

...ile/activities/providermanagerdashboard/addprovider/MatchingProviderRecyclerViewAdapter.java

// TODO open provider dashboard for clicked provider
holder.providerDetailsCL.setOnClickListener(v -> {
// Action
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Go ahead and write this implementation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will open the Provider-Patient relationship dashboard which will be implemented next. Currently we don't have provider dashboard.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, you're right. @deepak140596 file up a new issue for this and ping me there.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually AC-624 addresses this issue.

Copy link
Collaborator

@f4ww4z f4ww4z Jul 10, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@deepak140596 I was referring to creating the provider dashboard. I went ahead and created the issue, now it's ready for work :)

@deepak140596
Copy link
Contributor Author

deepak140596 commented Jul 8, 2019 •
edited
Loading

Screenshot_20190708-203243

I have added a stroked outline to the providers. Adding colors to the background is not looking good

Screenshot_20190708-203834

@deepak140596 deepak140596 reopened this Jul 8, 2019
@deepak140596
Copy link
Contributor Author

@f4ww4z , please review the changes. Also the builds have passed.

@f4ww4z
Copy link
Collaborator

f4ww4z commented Jul 9, 2019

@deepak140596 that kind of stroked outline is used for input fields in the app, so we can't use it here.

@deepak140596 See the list view here. Please change the similar providers dialog to be like that. Make sure to center the view in the box.

@deepak140596
Copy link
Contributor Author

@f4ww4z , as both the CardView and AlertDialog have the same white background as default, I changed the CardView's background as light shade of grey to distinguish between background and foreground.

Capture+_2019-07-10-00-55-16

f4ww4z
f4ww4z requested changes Jul 10, 2019
View reviewed changes
Copy link
Collaborator

@f4ww4z f4ww4z left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@deepak140596 The weight of the card view is too much. A white color is fine, no need to change it. Follow this style, it will look much better.

openmrs-client/src/main/res/layout/row_matching_provider.xml Outdated
<LinearLayout
android:layout_width="match_parent"
android:layout_height="wrap_content"
android:background="#EBEAEA">
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you see color.xml?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have removed this part of the code as we are using the default color. Also do I need to remove the Card View?

@deepak140596
Copy link
Contributor Author

deepak140596 commented Jul 10, 2019 •
edited
Loading

Capture+_2019-07-11-02-24-07

@f4ww4z ,with white color, the look will be like this.

f4ww4z
f4ww4z approved these changes Jul 11, 2019
View reviewed changes
Copy link
Collaborator

@f4ww4z f4ww4z left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work @deepak140596 , now the dialog looks materialized.

@f4ww4z f4ww4z merged commit d809cd7 into openmrs:master Jul 11, 2019
@deepak140596 deepak140596 deleted the AC-625 branch July 13, 2019 14:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@f4ww4z f4ww4z f4ww4z approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@deepak140596 @codecov-io @f4ww4z