OCLOMRS-33: Implement fetching concepts and search for concepts. #29

hadijahkyampeire · 2018-06-11T21:09:45Z

JIRA TICKET NAME:

OCLOMRS-33: Implement UI and logic for retrieving all concepts and search for specific concepts.

Summary:

This PR does the following:

Adds UI for viewing all concepts in OCL
Implements the logic for retrieving all concepts and searching through them
Adds tests for all the functionality

coveralls · 2018-06-11T21:22:48Z

Pull Request Test Coverage Report for Build 139

46 of 56 (82.14%) changed or added relevant lines in 8 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+2.8%) to 65.916%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/components/dashboard/container/Concepts.jsx	14	15	93.33%
src/redux/actions/concepts/index.js	9	18	50.0%

Totals
Change from base Build 33:	2.8%
Covered Lines:	164
Relevant Lines:	225

💛 - Coveralls

EleisonC

In concepts.test.jsx file do you mind removing the last empty space
Line 59

emasys · 2018-06-12T07:20:19Z

src/components/dashboard/components/SideNavigation.jsx

          Sources
        </a>
+        <a href="/dashboard/concepts" className="nav-link active">
+          <i className="fa fa-tags" /> Concepts
+        </a>
      </li>
    </ul>
  </div>


Is there a reason why you used the html anchor tag a instead of react-router Link?

'Link' was bringing some errors that I should not use 'Link' inside Route, however in React Link is always changed to anchor, so no harm

the regular html a tag would trigger a full page re-render, Link wouldn't. You might want to put that into consideration.

emasys · 2018-06-12T07:21:18Z

src/components/dashboard/components/concepts/ConceptCard.jsx

@@ -0,0 +1,29 @@
+import React from 'react';
+// import Avatar from 'react-avatar';
+// import moment from 'moment';


You should delete this commented code since they are no longer useful

on it thank you

emasys · 2018-06-12T07:22:28Z

src/redux/actions/concepts/ConceptActionCreators.js

@@ -0,0 +1,18 @@
+import { IS_FETCHING, FETCH_CONCEPTS } from '../types';
+
+// const FETCH_CONCEPTS = 'fetch_concepts';


delete this

shakirandagire · 2018-06-12T08:26:05Z

src/components/dashboard/components/concepts/ConceptCard.jsx

+// import moment from 'moment';
+
+
+const ConceptCard = (concept) => {


The name of the component is ConceptCard yet the content is a table. Could you consider changing the name of the component to indicate table instead of card because it is misleading.

Good point, on it, thanks

shakirandagire · 2018-06-12T08:27:42Z

src/components/dashboard/components/concepts/Search.jsx

+import React from 'react';
+import Tooltip from 'react-tooltip';
+
+/* eslint-disable */


Is there a relevance for this comment?

Let look into it

shakirandagire · 2018-06-12T08:29:22Z

src/components/dashboard/components/concepts/ConceptList.jsx

+        <tbody>
+
+          {concepts.map(concept => (
+            <Card concept={concept} key={concept.uuid} />


Could you change the component name from card to table to avoid confusion

mwaz · 2018-06-12T09:32:06Z

.eslintrc.json

@@ -7,6 +7,7 @@
    "jsx-a11y/label-has-for":0,
    "react/jsx-closing-tag-location":0,
    "import/no-named-as-default":0,
+    "import/prefer-default-export":0,


i think this rule should be there rather than being ignored by the linter , and if at viable for change all then should be made in a different PR

Let me look into it thanks

hadijahkyampeire · 2018-06-12T10:37:23Z

Hello Team, thanks for all the reviews, I have fixed all of them

shakirandagire · 2018-06-12T10:48:51Z

src/redux/reducers/ConceptReducers.js

@@ -0,0 +1,20 @@
+// import { FETCH_CONCEPTS } from '../actions/concepts/ConceptActionCreators';


Could you delete this commented out code.

EleisonC

Is there a reason you opted to use inline styling in ConceptTable.jsx and ConceptList.jsx

mwaz · 2018-06-12T12:06:16Z

src/components/dashboard/components/SideNavigation.jsx


+/* eslint-disable */


why do we have this comment?

Some statement that helps disable some funny rule

shakirandagire

LGTM

…ode" (openmrs#22) * Revert "Revert "OCLOMRS-29: Restructure app to improve code maintainability" (openmrs#21)" This reverts commit f6d6d28. * Revert "Revert "OCLOMRS-17: Protect routes that require authentication (openmrs#12)" (openmrs#20)" This reverts commit f6be69c. * Revert "OCLOMRS-17: Protect routes that require authentication (openmrs#12)" This reverts commit a0444d4. * Revert "OCLOMRS-29:Restructure app to improve code maintainability (openmrs#19)" This reverts commit e77aa4d. * Revert "OCLOMRS-21: Integrate Linting on CI and Prettier for better code (openmrs#17)" This reverts commit 7e68e2f. Revert "Revert "OCLOMRS-21: Integrate Linting on CI and Prettier for better code" (openmrs#22)" (openmrs#23) This reverts commit 5fcea0d. Revert "Revert "OCLOMRS-29: Restructure app to improve code maintainability"" (openmrs#24) * Revert "Revert "Revert "OCLOMRS-21: Integrate Linting on CI and Prettier for better code" (openmrs#22)" (openmrs#23)" This reverts commit f2f81b0. * Revert "Revert "OCLOMRS-21: Integrate Linting on CI and Prettier for better code" (openmrs#22)" This reverts commit 5fcea0d. * Revert "Revert "OCLOMRS-29: Restructure app to improve code maintainability" (openmrs#21)" This reverts commit f6d6d28. Revert "Revert "OCLOMRS-17: Protect routes that require authentication"" (openmrs#25) * Revert "Revert "Revert "OCLOMRS-29: Restructure app to improve code maintainability"" (openmrs#24)" This reverts commit 20ecf9b. * Revert "Revert "Revert "OCLOMRS-21: Integrate Linting on CI and Prettier for better code" (openmrs#22)" (openmrs#23)" This reverts commit f2f81b0. * Revert "Revert "OCLOMRS-21: Integrate Linting on CI and Prettier for better code" (openmrs#22)" This reverts commit 5fcea0d. * Revert "Revert "OCLOMRS-29: Restructure app to improve code maintainability" (openmrs#21)" This reverts commit f6d6d28. * Revert "Revert "OCLOMRS-17: Protect routes that require authentication (openmrs#12)" (openmrs#20)" This reverts commit f6be69c. OCLOMRS-35: display sources and enable searching (openmrs#26) OCLOMRS-40: redesign sources page user interface (openmrs#27)

fix eslint errors Add conceptActiontests Work on comments fix tests Remove white space Remove comment Remove comment Fix tests remove comment

hadijahkyampeire · 2018-06-14T07:51:34Z

Hello Team, this PR has been closed because of many merge conflicts that can't be solved but I am going to raise a new one by end of today. cc @dkayiwa @EleisonC

hadijahkyampeire force-pushed the OCLOMRS-33 branch 2 times, most recently from 0004285 to c00e53d Compare June 11, 2018 21:44

EleisonC suggested changes Jun 12, 2018

View reviewed changes

emasys reviewed Jun 12, 2018

View reviewed changes

shakirandagire reviewed Jun 12, 2018

View reviewed changes

mwaz reviewed Jun 12, 2018

View reviewed changes

hadijahkyampeire force-pushed the OCLOMRS-33 branch from 14ec932 to e61912a Compare June 12, 2018 10:35

shakirandagire reviewed Jun 12, 2018

View reviewed changes

hadijahkyampeire force-pushed the OCLOMRS-33 branch 2 times, most recently from f2c1bc9 to 5679791 Compare June 12, 2018 11:43

EleisonC reviewed Jun 12, 2018

View reviewed changes

mwaz reviewed Jun 12, 2018

View reviewed changes

hadijahkyampeire force-pushed the OCLOMRS-33 branch from 7c6c705 to 6a03719 Compare June 12, 2018 13:46

shakirandagire approved these changes Jun 12, 2018

View reviewed changes

hadijahkyampeire force-pushed the OCLOMRS-33 branch 2 times, most recently from f0b804e to 52ed74e Compare June 12, 2018 18:44

dkayiwa and others added 2 commits June 12, 2018 22:12

OCLOMRS-33:Implement fetching concepts and search for concepts.

4dca24b

fix eslint errors Add conceptActiontests Work on comments fix tests Remove white space Remove comment Remove comment Fix tests remove comment

hadijahkyampeire force-pushed the OCLOMRS-33 branch 2 times, most recently from 795ab6f to 4dca24b Compare June 12, 2018 19:15

Resolve conflicts

2a4b8a1

hadijahkyampeire closed this Jun 14, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OCLOMRS-33: Implement fetching concepts and search for concepts. #29

OCLOMRS-33: Implement fetching concepts and search for concepts. #29

hadijahkyampeire commented Jun 11, 2018

coveralls commented Jun 11, 2018 •

edited

Loading

EleisonC left a comment •

edited

Loading

emasys Jun 12, 2018

hadijahkyampeire Jun 12, 2018

emasys Jun 12, 2018

emasys Jun 12, 2018

hadijahkyampeire Jun 12, 2018

emasys Jun 12, 2018

hadijahkyampeire Jun 12, 2018

shakirandagire Jun 12, 2018

hadijahkyampeire Jun 12, 2018

shakirandagire Jun 12, 2018

hadijahkyampeire Jun 12, 2018

shakirandagire Jun 12, 2018

hadijahkyampeire Jun 12, 2018

mwaz Jun 12, 2018 •

edited

Loading

hadijahkyampeire Jun 12, 2018

hadijahkyampeire commented Jun 12, 2018

shakirandagire Jun 12, 2018

EleisonC left a comment

mwaz Jun 12, 2018

hadijahkyampeire Jun 12, 2018

shakirandagire left a comment

hadijahkyampeire commented Jun 14, 2018

		@@ -0,0 +1,18 @@
		import { IS_FETCHING, FETCH_CONCEPTS } from '../types';

		// const FETCH_CONCEPTS = 'fetch_concepts';

		// import moment from 'moment';


		const ConceptCard = (concept) => {

		@@ -0,0 +1,20 @@
		// import { FETCH_CONCEPTS } from '../actions/concepts/ConceptActionCreators';

OCLOMRS-33: Implement fetching concepts and search for concepts. #29

OCLOMRS-33: Implement fetching concepts and search for concepts. #29

Conversation

hadijahkyampeire commented Jun 11, 2018

JIRA TICKET NAME:

Summary:

coveralls commented Jun 11, 2018 • edited Loading

Pull Request Test Coverage Report for Build 139

💛 - Coveralls

EleisonC left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mwaz Jun 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hadijahkyampeire commented Jun 12, 2018

Choose a reason for hiding this comment

EleisonC left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shakirandagire left a comment

Choose a reason for hiding this comment

hadijahkyampeire commented Jun 14, 2018

coveralls commented Jun 11, 2018 •

edited

Loading

EleisonC left a comment •

edited

Loading

mwaz Jun 12, 2018 •

edited

Loading