Closed santam85 closed 2 years ago
santam85
commented
3 years ago @benmccann can you have another look at the latest iteration of this? I'd like it to be merged soon, since as things are standing now, the master branch would not even build for me on the latest Chart.js with the following error:
Error: ./node_modules/chartjs-chart-financial/src/controller.candlestick.js 47:3-8
"export 'default' (imported as 'Chart') was not found in 'chart.js'
benmccann
commented
3 years ago This PR is getting quite difficult to review as more changes are tacked on to it. I think most of the changes are good, but it's a bit hard for me to even tell what's changed especially with the indentation changed. Would you be able to split this up into separate PRs?
santam85
commented
3 years ago @benmccann enforced tab indentation in eslint so that those changes are not confusing you and won't be added inadvertently in the future. Unfortunately it's quite difficult to split these changes up in a way they are self consistent: the changes required for ESM are pervasive.
benmccann
commented
3 years ago I do think it could be split up a bit. I just pushed two of the changes separately. If you rebase now hopefully it should make this PR at least a bit smaller
santam85
commented
3 years ago I do think it could be split up a bit. I just pushed two of the changes separately. If you rebase now hopefully it should make this PR at least a bit smaller
Just did. Please add comments on whatever needs clarification or changes. I'd really rater not split this, as I need all these changes in order to continue work on ng2-charts and I would hope we can avoid delaying this further.
santam85
commented
3 years ago @benmccann any new comments? Do you want me to update the README and the docs too?
Eugeno
commented
3 years ago Are there any updates?
santam85
commented
3 years ago @benmccann I'd like some closure on this PR in order to understand if we need to remove the financial charts plugin from ng2-charts. If you or any other maintainer could give a final review to the PR and tell us what needs to be addressed, I'd be very grateful. Otherwise please reject and close.
dostarora97
commented
2 years ago @benmccann Any updates on this PR ? I believe quite a lot of people depend on this PR, since its' incorporated in ng2-charts. Could you plz provide us any updates ?
ameryousuf
commented
2 years ago @benmccann any update on this PR, please?
santam85
commented
2 years ago @etimberg @kurkle Perhaps you can be of help here?
kurkle
commented
2 years ago Thanks for the patience, @santam85 I think it will take at least couple of weeks before @etimberg is available, no idea how busy @benmccann is nowadays. I would not like to step on any toes by just merging this.
benmccann
commented
2 years ago thank you for this! this is a really nice set of changes and looks good to me as well. I just left two minor comments along with the one @kurkle left. if you don't mind making those minor updates we can go ahead and merge this
sorry for being MIA on this. I kept meaning to get back to it. thanks for being persistent while being patient and understanding at the same time
santam85
commented
2 years ago thank you for this! this is a really nice set of changes and looks good to me as well. I just left two minor comments along with the one @kurkle left. if you don't mind making those minor updates we can go ahead and merge this
sorry for being MIA on this. I kept meaning to get back to it. thanks for being persistent while being patient and understanding at the same time
No problem, we're all busy with our day jobs 😄 Applied the new suggestions, let me know if there's more.
santam85
commented
2 years ago fixes #99
Thanks for contributing.
Description
Updated Chart.js peer dependency to the latest version, and fixed broken tests. Added ESM build to avoid warnings when using the plugin with Angular (https://github.com/valor-software/ng2-charts/pull/1276)