-
Notifications
You must be signed in to change notification settings - Fork 105
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
feat(component) bh-bebtor-group-history #3611
feat(component) bh-bebtor-group-history #3611
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jeremielodi this is a good start!
It looks like a mistake happened in the Patient Registration page, let's restore it back to the old values.
Could you put this component on the patient edit page? Right under this:
That way we can see how it looks!
transclude : true, | ||
bindings : { | ||
debtorUuid : '@?', | ||
limit : '<', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this should be optional.
controller : bhDebtorGroupHistoryController, | ||
transclude : true, | ||
bindings : { | ||
debtorUuid : '@?', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like the debtorUuid
should be required.
$ctrl.$onInit = () => { | ||
$ctrl.limit = $ctrl.limit || 5; | ||
loadHistory(); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's also put a $onChanges
binding so that we reload the history when the debtorUuid
changes.
@@ -117,5 +118,11 @@ function DebtorGroupService(Modal, Session, $translate, Api) { | |||
.then(service.util.unwrapHttpResponse); | |||
} | |||
|
|||
function history(debtorUuid, limit) { | |||
const url = `${baseUrl}history/${debtorUuid}/${limit}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you make the limit
into a query string? That way, it would be option and you could either see the entire history with /history/:uuid
or a slice of the history with /history/:uuid?limit=X
.
@@ -101,7 +92,7 @@ | |||
<div class="help-block text-right"> | |||
<p> | |||
<a ng-click="PatientRegCtrl.toggleFullDate()" href> | |||
<i class="fa fa-circle-o" ng-class="PatientRegCtrl.dateIndicatorIcon"></i> | |||
<i class="fa fa-calendar-o"></i> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this might change the display @mbayopanda was going for - in one state, it is supposed to be a circle that is filled in and in the other it is supposed to be a circle that is empty.
</ol> | ||
|
||
<div class="toolbar"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think all the changes to this page must be a mistake. Can you revert them?
<p><u><span translate>DEBTOR_GROUP.CHANGE</span></u></p> | ||
|
||
<ul class="list-unstyled"> | ||
<li ng-repeat="change in $ctrl.groupChanges | orderBy:'-created_at'"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since you do the ORDER BY in SQL, there isn't a need for this here.
<ul class="list-unstyled"> | ||
<li ng-repeat="change in $ctrl.groupChanges | orderBy:'-created_at'"> | ||
<i class="fa fa-clock-o"></i> | ||
<b><span translate>FORM.LABELS.FROM</span></b> {{change.group_prev}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's make this into an arrow to make it super clear: Something like
{{change.group_prev}} → {{change.group_next}}
which will render → as the arrow.
940bee6
to
9562db0
Compare
@@ -117,5 +118,11 @@ function DebtorGroupService(Modal, Session, $translate, Api) { | |||
.then(service.util.unwrapHttpResponse); | |||
} | |||
|
|||
function history(debtorUuid, parameters) { | |||
const url = `${baseUrl}history/${debtorUuid}`; | |||
return service.$http.get(url, { params : parameters || {}}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A space is required before '}' object-curly-spacing
} | ||
|
||
function loadHistory() { | ||
const parameters = { limit : $ctrl.limit }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Multiple spaces found before '{' no-multi-spaces
|
||
$ctrl.$onChanges = () => { | ||
loadHistory(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing semicolon semi
9562db0
to
c6e24f5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Good work.
bors r+
Build succeeded |
closes #3609