Skip to content

[IMPAC-758] Date hidden in X-axis for current ratio and debt service widget#456

Closed
MAhsenArif wants to merge 3 commits intomaestrano:release/v1.6.9from
MAhsenArif:fix_hidden_date_issue
Closed

[IMPAC-758] Date hidden in X-axis for current ratio and debt service widget#456
MAhsenArif wants to merge 3 commits intomaestrano:release/v1.6.9from
MAhsenArif:fix_hidden_date_issue

Conversation

@MAhsenArif
Copy link
Copy Markdown
Contributor

No description provided.

Copy link
Copy Markdown
Contributor

@xaun xaun left a comment

Choose a reason for hiding this comment

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

Thanks for your PR @MAhsenArif. Just a few minor change suggestions, thanks.

$scope.intervalsCount = 0
$scope.isPnl = false
$scope.periodInfoContext = {}
$scope.allowXAxisLegend = true
Copy link
Copy Markdown
Contributor

@xaun xaun Feb 1, 2018

Choose a reason for hiding this comment

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

No need to initialise the property here as well as in the w.initContext initializer, as it makes sense for it to stay hidden until it has been validated in the initContext block.. rather than for it to show, and then a few milliseconds later, for it to be hidden.

$scope.calculatedNumerator = _.last(w.content.calculation.numerator.values)
$scope.calculatedDenominator = _.last(w.content.calculation.denominator.values)

$scope.allowWidgetLegend = ->
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This method is not being used in the view, so it should not be bound to $scope.

$scope.periodInfoContext.histParams = w.metadata.hist_parameters
$scope.periodInfoContext.accountingBehaviour = behaviour
getPrefix(behaviour).then((label)-> $scope.periodInfoContext.injectBefore = label)
$scope.allowXAxisLegend = $scope.allowWidgetLegend()
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

As this method is only being invoked here, I think we can just do this:

$scope.allowXAxisLegend = !_.includes(['accounts/ratios/current', 'accounts/ratios/debt_service'], w.endpoint)

This leverages Lodash, to shorten & add readability to the statement, and then we can remove the allowWidgetLegend method.

<div class="history chart-container" ng-show="widget.isHistoryMode">
<div impac-chart draw-trigger="::drawTrigger.promise" deferred="::chartDeferred"></div>
<div class="legend">{{ widget.content.layout.ratio }}</div>
<div ng-show="allowXAxisLegend" class="legend">{{ widget.content.layout.ratio }}</div>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Prefer ng-if over ng-show. ng-show is really only useful if you want to specifically want to keep an element in the DOM.

@xaun xaun self-assigned this Feb 1, 2018
@MAhsenArif MAhsenArif force-pushed the fix_hidden_date_issue branch from 177bbae to b85818a Compare February 2, 2018 07:16
Copy link
Copy Markdown
Contributor

@xaun xaun left a comment

Choose a reason for hiding this comment

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

Thanks

@xaun xaun changed the base branch from staging to release/v1.6.9 February 5, 2018 12:21
@xaun
Copy link
Copy Markdown
Contributor

xaun commented Feb 5, 2018

Merged in #472 to fix conflicts

@xaun xaun closed this Feb 5, 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

Development

Successfully merging this pull request may close these issues.

3 participants