Skip to content

[IMPAC-618] Better time management for cashflow widgets#397

Merged
cesar-tonnoir merged 10 commits intomaestrano:release/v1.6.1from
cesar-tonnoir:feature/time-management
Sep 11, 2017
Merged

[IMPAC-618] Better time management for cashflow widgets#397
cesar-tonnoir merged 10 commits intomaestrano:release/v1.6.1from
cesar-tonnoir:feature/time-management

Conversation

@cesar-tonnoir
Copy link
Copy Markdown
Contributor

@cesar-tonnoir cesar-tonnoir commented Sep 2, 2017

  • Uses highstock instead of highchart
  • Build labels based on timestamps returned with each series instead of dates strings
  • Better zooming capabilities
  • Change time period using dates pickers
  • Better granularity (use daily intervals)

Goes with https://github.com/maestrano/impac-finance-bolt/pull/44

voila_capture 2017-09-03_2-30-45_am

TODO

  • Fix threshold tooltip
  • Fix threshold panel appears when using the zoom shortcuts

@cesar-tonnoir cesar-tonnoir changed the title Feature/time management [IMPAC-618] Better time management for cashflow widgets Sep 2, 2017
@cesar-tonnoir cesar-tonnoir removed the WIP label Sep 2, 2017
@cesar-tonnoir cesar-tonnoir requested a review from xaun September 2, 2017 16:27
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.

Looks good, just a few comments.

}
span {
outline: none;
}
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.

I fixed this in feature/threshold-tooltips (cbdf27b). I think I prefer my changes, but I guess you'll be resolving the conflicts so you can pick :)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

👍
cherry-picked your commit

# Widget histParams are YTD by default (when undefined on metadata),
# therefore in the past by default
ctrl.disabled = _.isEmpty(widgetHistParams) || moment(widgetHistParams.to) <= moment().startOf('day')
ctrl.disabled = widgetHistParams? && moment(widgetHistParams.to) <= moment.utc().startOf('day')
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.

if widgetHistParams is equal to undefined or null, ctrl.disabled will equal false. Is this intended?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yes: I disable it only if I have hist parameters defined so that the time period is in the past.
the default time period is no longer YTD but -3m / +1m

chartConfig = angular.merge({}, @template(), @formatters(), @todayMarker())
if _.isEmpty(@hc)
@hc = Highcharts.chart(@id, chartConfig)
@hc = Highcharts.stockChart(@id, chartConfig)
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.

Will all Impac! angular charts use StockChart now?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

If I'm not wrong, stockChart is just like highCharts with more options, so I think this should be ok?..

@cesar-tonnoir cesar-tonnoir merged commit 21d2705 into maestrano:release/v1.6.1 Sep 11, 2017
@cesar-tonnoir cesar-tonnoir deleted the feature/time-management branch September 11, 2017 07:55
@cesar-tonnoir cesar-tonnoir mentioned this pull request Sep 11, 2017
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants