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

OCLOMRS-84: Redesign user dashboard #74

Merged
merged 1 commit into from
Jul 20, 2018
Merged

Conversation

emasys
Copy link
Contributor

@emasys emasys commented Jul 18, 2018

JIRA TICKET NAME:

Redesign the user dashboard to follow the provided mockup

Summary:

As a user, after signing in to the app, I should see a user dashboard similar to what is in the mockup

@coveralls
Copy link

coveralls commented Jul 18, 2018

Pull Request Test Coverage Report for Build 388

  • 74 of 75 (98.67%) changed or added relevant lines in 6 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+1.2%) to 82.101%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/components/userDasboard/container/UserDashboard.jsx 8 9 88.89%
Totals Coverage Status
Change from base Build 371: 1.2%
Covered Lines: 850
Relevant Lines: 973

💛 - Coveralls

@emasys emasys force-pushed the OCLOMRS-84 branch 2 times, most recently from 3915336 to 209994c Compare July 18, 2018 12:04
Copy link
Collaborator

@hadijahkyampeire hadijahkyampeire left a comment

Choose a reason for hiding this comment

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

LGTM, nice rework

numberOfDictionary: PropTypes.number.isRequired,
organizations: PropTypes.array.isRequired,
};

Copy link

@Efosaok Efosaok Jul 18, 2018

Choose a reason for hiding this comment

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

This component is rendering three different views based on some condition, don't you think it'll be better if these three different views are modularised as it would aid reusability and make this component look more compact.

Each possible view in this component has the div tag with className user-data and a p tag with className lead, I think it'll be better if you write this without having to repeat this tags in the different views.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your feedback. However, if you closely look at the component you'd see there's no point modularizing any part of this component. Inasmuch as all the blocks look similar I would like you to take a closer look at the lines below:

  • 12
  • 26
  • 58

Trying to break this component furthermore will only make it more difficult to understand or edit in the future.

Thanks again. @Efosaok

Copy link

Choose a reason for hiding this comment

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

sorry I forgot to approve this earlier, I just felt it would be better modularizing this component, but if you insist it's better this way for this project, it's fine as well. as for the templating on those lines, correct me if I am wrong, but I do not see how modularizing it affects that part. the child component could receive the value as props.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My point exactly, I don't think that is necessary. Thanks a lot.

Copy link

@Efosaok Efosaok left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@EleisonC EleisonC left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@shakirandagire shakirandagire left a comment

Choose a reason for hiding this comment

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

LGTM

@dkayiwa dkayiwa merged commit 4b4b257 into openmrs:master Jul 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
7 participants