adobe / franklin-dashboard

Apache License 2.0
4 stars 2 forks source link

feat(chart.js): make basis for future work #12

Closed MarquiseRosier closed 11 months ago

MarquiseRosier commented 11 months ago

this commit will start us with a graphing technology, as well as a way of tying different charts and different data to a block

Please always provide the GitHub issue(s) your PR is for, as well as test URLs where your change can be observed (before and after):

Fix #

Test URLs:

aem-code-sync[bot] commented 11 months ago

Hello, I'm Franklin Bot and I will run some test suites that validate the page speed. In case there are problems, just click the checkbox below to rerun the respective action.

langswei commented 11 months ago

Good enough for initial merge to the other repo from my perspective, just two suggestions for now:

  1. For engineer-data.js, I would suggest more manipulation in sql and less in javascript.
  2. For engineer-dom.js, it reads like every instance of the block will require a code change. I don't know if I'm reading it correctly or not, and I understand your comments on our call earlier about having extension points but I think extensions should be reserved for exceptions rather than all instances.