refactor templated JS #46
Conversation
| @@ -15,6 +15,9 @@ | |||
|
|
|||
| <script type="text/javascript" src="https://cdnjs.cloudflare.com/ajax/libs/Chart.js/2.7.2/Chart.min.js"></script> | |||
| <script type="text/javascript" src="https://cdnjs.cloudflare.com/ajax/libs/cash/3.0.0-beta.3/cash.min.js"></script> | |||
| <script> | |||
| window.AUDIT = {{ .JSON }}; | |||
There was a problem hiding this comment.
I think it would be good to use a Fairwinds prefixed name for a global variable like this, maybe even just FairwindsAuditData or something?
public/js/charts.js
Outdated
| labels: Object.keys(AUDIT.NamespacedResults), | ||
| datasets: [{ | ||
| label: 'Passing', | ||
| data: Object.values(AUDIT.NamespacedResults).map(r => r.Summary.Successes), |
There was a problem hiding this comment.
It sure is nice to not have to target any legacy browsers
There was a problem hiding this comment.
Probably worth being safe though. I think Safari still struggles with arrow functions.
I'm so used to working with transpilers that I often forget about browser support.
6c46ddd to
0c6b644
Compare
| labels: Object.keys(fairwindsAuditData.NamespacedResults), | ||
| datasets: [{ | ||
| label: 'Passing', | ||
| data: Object.values(fairwindsAuditData.NamespacedResults) |
There was a problem hiding this comment.
Even Object.values is a bit of a stretch for older browsers unfortunately: https://caniuse.com/#feat=object-values. I don't feel too strongly on this, there's probably not a ton of overlap between people using this tool and people using old browsers, but it certainly could be an issue.
There was a problem hiding this comment.
If it's a pain to reimplement that, you could also include all or part of lodash: https://www.jsdelivr.com/package/npm/lodash
There was a problem hiding this comment.
Good call - wrote a quick polyfill
0c6b644 to
dab34b9
Compare
This takes the audit data and creates a single variable using JSON, rather than using templating to create JavaScript on the fly.