isotope / core

Core repository of Isotope eCommerce, an eCommerce extension for Contao Open Source CMS
https://isotopeecommerce.org
136 stars 107 forks source link

switch first report member guests to apexcharts lib #2483

Closed error08 closed 1 year ago

error08 commented 1 year ago

My suggestion to replace the outdated xchart library with the modern apexcharts library.

With this PR I only changed the report MemberGuests.

Next I would change the SalesTotal, then the D3 Lib and the xcharts can be removed.

If this fits, the next PR will follow soon. In addition, I plan to include the conversation rate in sales.

aschempp commented 1 year ago

Great enhancement, thanks a lot! Can we update the initializeChart method instead of transforming the data for apexcharts? And could you rebase the pull request on Isotope 2.9 (or I can do that)?

Also I would prefer to remove the unnecessary plugin files and fix some code style. Let me know if I should apply that or you will.

error08 commented 1 year ago

Sure! I will refactor the initializeChart function and rebase the PR to 2.9. When I have trouble with the rebasing, I will contact you.

error08 commented 1 year ago

I had no idea how to more simplify the initializeChart function. For grouping the data, the time range is relevant. Later for the chart generation it is no longer necessary.

error08 commented 1 year ago

"Also I would prefer to remove the unnecessary plugin files and fix some code style. Let me know if I should apply that or you will."

Yes sure! But with my next PR regarding sales_total. At the moment sales_total still requires the old libraries.

aschempp commented 1 year ago

Thanks a lot! I did mean to remove unnecessary files from the apexcharts folder, since we only seem to use one JS file (and probably some CSS), not all of the AMD stuff or the locales. Please also remove the export menu at the top, I don't think that makes sense on the chart (and it would mean we really don't need locales 😅 )

error08 commented 1 year ago

I not sure if the lib works correct without the locale files. In my tests it seems it works.

error08 commented 1 year ago

Please wait for merging the PR, I found an error in the weekly and daily view. Here is a difference between the chart and table. The bug seems to have existed for a long time.

Because of that line

$intStart = strtotime('first day of this month', $intStart);

in the function initializeChart the chart and the table starts with different time range. (e.g. day or week) Is there a specific reason for?

aschempp commented 1 year ago

in the function initializeChart the chart and the table starts with different time range. (e.g. day or week) Is there a specific reason for?

I honestly have no idea, it has been at least 5 years since I touched that 😂 It's not about monday or sunday start of the week, right?

error08 commented 1 year ago

I removed it. Now I think the PR is complete.

aschempp commented 1 year ago

Thank you @error08 !