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 from 2.9.4 to 3.9.1. #2373

Merged
merged 13 commits into from
Oct 5, 2022
Merged

Update chart.js from 2.9.4 to 3.9.1. #2373

merged 13 commits into from
Oct 5, 2022

Conversation

yubiuser
Copy link
Member

@yubiuser yubiuser commented Sep 23, 2022

What does this PR aim to accomplish?:

Update chart.js from 2.9.4 to 3.9.1.
Needed to add chartjs-adapater-moment.js. I tried to be as close as possible to the old visual style. A few unnecessary options were removed, some defaults were explicitly set (e.g. responsive: true).

Useful for review: https://www.chartjs.org/docs/3.4.1/getting-started/v3-migration.html


By submitting this pull request, I confirm the following:

  1. I have read and understood the contributors guide, as well as this entire template. I understand which branch to base my commits and Pull Requests against.
  2. I have commented my proposed changes within the code and I have tested my changes.
  3. I am willing to help maintain this change if there are issues with it later.
  4. It is compatible with the EUPL 1.2 license
  5. I have squashed any insignificant commits. (git rebase)

  • I have read the above and my PR is ready for review. Check this box to confirm

@yubiuser yubiuser force-pushed the chartjs branch 4 times, most recently from 60fee99 to f5a5033 Compare September 28, 2022 11:43
@yubiuser yubiuser added Requires Testing PR: Code Review Required Open Pull Request, needs code reviewed labels Sep 28, 2022
@yubiuser yubiuser force-pushed the chartjs branch 3 times, most recently from 0f6e044 to 31e89dd Compare October 1, 2022 07:55
@tingreavinash
Copy link

Hi @yubiuser,

I want to contribute to AdminLTE project, can you please help with local setup (Windows) of this project?
I have installed NPM, Cloned this repo, npm install, but not able to run the project ..

@yubiuser
Copy link
Member Author

yubiuser commented Oct 1, 2022

Do you have a webserver installed?

@tingreavinash
Copy link

Do you have a webserver installed?

@yubiuser, Yes, I have xampp server..

@yubiuser
Copy link
Member Author

yubiuser commented Oct 1, 2022

The webinterface is tightly coupled with the Pi-hole binary FTL which provides all the data. This binary doesn't run on Windows, but Linux. You could surely make changes to the interface without having a working Pi-hole instance, but usually your better of if you have the complete setup. I run Linux myself and edit the files directly within the web dir /var/www/html/admin which is a git clone of the repo.
I know @rdwebdesign is using Windows (not 100% sure for development), maybe he could explain his setup.

@rdwebdesign rdwebdesign force-pushed the chartjs branch 5 times, most recently from 44e69a2 to 999a67a Compare October 4, 2022 18:48
@yubiuser yubiuser force-pushed the chartjs branch 2 times, most recently from 2624898 to d904cf4 Compare October 4, 2022 20:01
@yubiuser yubiuser marked this pull request as ready for review October 4, 2022 20:01
@yubiuser yubiuser requested a review from a team October 4, 2022 20:01
@DL6ER
Copy link
Member

DL6ER commented Oct 5, 2022

Thanks for this PR. I had only limited time for testing this morning but I'll try to return to it later. It looks good to me, the only issue I've seen so far is a misplacement of the loading spinners:
Screenshot from 2022-10-05 04-38-49

edit I've just seen that this is an issue on devel, too.

@rdwebdesign
Copy link
Member

rdwebdesign commented Oct 5, 2022

I also see the spinners on the same place in master.

image
I think the spinners are there since a long time ago.


Edit:
Before attempt any modifications, I was trying to understand where the spinners supposed to be placed and what happened.
I found out this was probably caused by this commit.

The spinner uses a fontawesome icon. This icon is replaced (and resized and repositioned) by script.
This change was made almost 2 years ago.

@yubiuser
Copy link
Member Author

yubiuser commented Oct 5, 2022

Spinner is fixed by #2385
Screenshot at 2022-10-05 09-16-52

DL6ER
DL6ER previously approved these changes Oct 5, 2022
Signed-off-by: Christian König <ckoenig@posteo.de>
Signed-off-by: Christian König <ckoenig@posteo.de>
Signed-off-by: Christian König <ckoenig@posteo.de>
yubiuser and others added 8 commits October 5, 2022 21:19
Signed-off-by: Christian König <ckoenig@posteo.de>
Signed-off-by: Christian König <ckoenig@posteo.de>
…ove it from the distributed version (chartjs/chartjs-adapter-moment#34) but kept the reference in the js file. I compiled it myself to generate the source map

Signed-off-by: Christian König <ckoenig@posteo.de>
Signed-off-by: Christian König <ckoenig@posteo.de>
Signed-off-by: Christian König <ckoenig@posteo.de>
Signed-off-by: RD WebDesign <github@rdwebdesign.com.br>
Signed-off-by: RD WebDesign <github@rdwebdesign.com.br>
…sect to false

Signed-off-by: Christian König <ckoenig@posteo.de>
…argins

Signed-off-by: Christian König <ckoenig@posteo.de>
…ess detailed tooltip on doughnut charts if nothing is hidden

Signed-off-by: Christian König <ckoenig@posteo.de>
@yubiuser yubiuser requested a review from a team October 5, 2022 20:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: Code Review Required Open Pull Request, needs code reviewed Requires Testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants