Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update chart.js to v2.9.3 and moment.js to v2.24 #1115

Merged
merged 1 commit into from
May 12, 2020
Merged

Conversation

XhmikosR
Copy link
Contributor

@XhmikosR XhmikosR commented Dec 29, 2019

Also, switch to the plain chart.js build since their bundle included moment.js, thus include moment before chart.

By submitting this pull request, I confirm the following: {please fill any appropriate checkboxes, e.g: [X]}

{Please ensure that your pull request is for the 'devel' branch!}

  • I have read and understood the contributors guide, as well as this entire template.
  • I have made only one major change in my proposed changes.
  • I have commented my proposed changes within the code.
  • I have tested my proposed changes.
  • I am willing to help maintain this change if there are issues with it later.
  • I give this submission freely and claim no ownership.
  • It is compatible with the EUPL 1.2 license
  • I have squashed any insignificant commits. (git rebase)
  • I have Signed Off all commits. (git commit --signoff)

This is untested

The chartjs bundle had moment.js bundled. This patch switches to the normal dist file and includes moment.js separately.

@XhmikosR XhmikosR force-pushed the update-chartjs branch 3 times, most recently from 3b6d915 to cbbc7d5 Compare January 3, 2020 10:30
@XhmikosR XhmikosR requested review from DL6ER and PromoFaux March 6, 2020 15:46
@XhmikosR XhmikosR marked this pull request as draft May 11, 2020 18:58
@XhmikosR XhmikosR marked this pull request as ready for review May 12, 2020 04:26
@DL6ER
Copy link
Member

DL6ER commented May 12, 2020

This changes the animation for the charts.

devel
ezgif-1-4aa28875e195

your branch
ezgif-1-1e4070ceb80d

@XhmikosR
Copy link
Contributor Author

Hmm, I'll change the changelog, some defaults might have changed.

This avoids including moment.js twice since chart.js bundle included moment.js

Signed-off-by: XhmikosR <[email protected]>
@XhmikosR
Copy link
Contributor Author

@DL6ER can you try again please? I downgraded to the same chart.js version, but kept the plain build so that we don't include moment.js 2x.

@DL6ER
Copy link
Member

DL6ER commented May 12, 2020

Else than my comment, everything seems to work.

However, why did you remove the conditional inclusion of moment.js? It is needed by only a small subset of pages and doesn't need to be loaded everywhere.

Furthermore, how does the update of moment.js work? You removed the bundle and added a new file only saying:

/*!
* Chart.js v2.9.3
* https://www.chartjs.org
* (c) 2019 Chart.js Contributors
* Released under the MIT License
*/

There is no mentioning about a contained moment.js. This is very unfortunate.

@XhmikosR
Copy link
Contributor Author

The previous chart.js bundle build included moment.js already. So you had it in all pages already.

https://www.chartjs.org/docs/latest/getting-started/installation.html#bundled-build

But we still included moment.js individually. This makes sure we don't load moment.js twice.

@XhmikosR
Copy link
Contributor Author

So, wait does the issue still happen with my latest push?

@DL6ER
Copy link
Member

DL6ER commented May 12, 2020

No. I'm just wondering that you removed

  • scripts/vendor/Chart.bundle.min.js
  • scripts/vendor/moment.min.js
    and added
  • scripts/vendor/Chart.min.js

Nothing is suggesting any more that moment is bundled into Chart.

If this is still the case, shouldn't the updated file be called Chart.bundle as well? This should also remove the adding/removing as the files seem identical:
Screenshot at 2020-05-12 22-14-23

@XhmikosR
Copy link
Contributor Author

I didn't remove moment.js. It's there, minified.

This patch is using the plan chart.js without moment.js, which we already had and just updated it.

Copy link
Member

@DL6ER DL6ER left a comment

Choose a reason for hiding this comment

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

I see, I got tricked by Github's interface which seems to suggest that moment.min.js was removed
Screenshot at 2020-05-12 22-21-27
while it was actually just shortened to one line.

@DL6ER DL6ER merged commit acc327a into devel May 12, 2020
@DL6ER DL6ER deleted the update-chartjs branch May 12, 2020 20:22
@XhmikosR XhmikosR added this to the v5.1 milestone May 29, 2020
@PromoFaux PromoFaux mentioned this pull request Jul 5, 2020
@pralor-bot
Copy link

This pull request has been mentioned on Pi-hole Userspace. There might be relevant details there:

https://discourse.pi-hole.net/t/pi-hole-5-1-released/35577/1

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