search

Codeinwp / visualizer

WordPress Visualizer plugin is a simple, easy to use and quite powerful tool to create, manage and embed interactive charts into your WordPress posts and pages.
102 stars 29 forks source link

Gutenberg: Add support for chartJS #619

Open DanyCaissy opened 4 years ago

DanyCaissy commented 4 years ago

Upon clicking this menu: https://i.imgur.com/5cCkdOh.png

Tables and google charts will be displayed in that menu, but chartjs charts are not displayed. They can only be included using the old tag.

As this behaviour is inconsistent it may confuse users.

contactashish13 commented 4 years ago

@DanyCaissy this should go into its own version (maybe 2 versions in the future) as it will monopolize all of my time and it should not hold back other releases.

DanyCaissy commented 4 years ago

@contactashish13 I'd like this to be addressed in this release as it is causing inconsistent behavior on an important feature of the plugin, and I think could arguably be considered a bug.

If there is really no way to fix this in a reasonable amount of time, at a minimum I'd want this to be investigated in detail as to why it cannot be added, and reviewed by another developer that is familiar with Gutenberg.

contactashish13 commented 4 years ago

Sure. Please ask Hardeep.

On Mar 12, 2020, 14:54, at 14:54, Dany Caissy notifications@github.com wrote:

@contactashish13 I'd like this to be addressed in this release as it is causing inconsistent behavior on an important feature of the plugin, and I think could arguably be considered a bug.

If there is really no way to fix this in a reasonable amount of time, at a minimum I'd want this to be investigated in detail as to why it cannot be added, and reviewed by another developer that is familiar with Gutenberg.

-- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: https://github.com/Codeinwp/visualizer/issues/619#issuecomment-598087302

DanyCaissy commented 4 years ago

@HardeepAsrani

Could you answer the following questions, in relation to this ticket?

HardeepAsrani commented 4 years ago

@DanyCaissy @contactashish13

DanyCaissy commented 4 years ago

@HardeepAsrani Thanks!

@contactashish13 Could you try and resolve this for this release? Even if it takes more development time I think this issue should get resolved sooner than later.

contactashish13 commented 4 years ago

@DanyCaissy I can't do it in 2-3 days. Maybe @HardeepAsrani can get 2-3 days over the next 2-3 weeks to get this done.

contactashish13 commented 4 years ago

@DanyCaissy let me know if Hardeep is doing this or I can do it as I see fit without externally enforced timelines.

DanyCaissy commented 4 years ago

@contactashish13 It's ok for me if it takes longer. I just wanted an idea on the feasibility and how big this task could be. Please work on it when you can and if it does end up taking too much time and holding back the release, or if there is a large blocker, we can consider moving this to a later release then.

DanyCaissy commented 4 years ago

@contactashish13 Has there been more progress on this issue? Do you have an ETA of when it could be completed?

contactashish13 commented 4 years ago

@DanyCaissy there has been some progress. Hopefully we can target to release this as part of the next (not current) release.

contactashish13 commented 4 years ago

@DanyCaissy also, as suggested a couple of weeks ago, it will be really instructive to use metabase to determine how many people actually use chartjs. From my perspective, it is a poorly implemented library and in all this time not one person has asked about it on support. It will tell us whether all this effort is even worth it or not.

DanyCaissy commented 4 years ago

@contactashish13 We can target it for next release, I'll see about removing some other features for the next release to account for that. For metabase, I'll take a look at these stats this week.

contactashish13 commented 4 years ago

PR https://github.com/Codeinwp/visualizer/pull/666 is ready for testing.

I've tried to make sure the same settings are seen in both the library and block editor but it would be good to verify and list down what is missing.

One known bug is when a bar/column chart is made stacked, it cannot be unstacked.

DanyCaissy commented 4 years ago

@contactashish13

Overall it looks pretty good, nice work on this feature. The stacked bug is unfortunate but we can live with that. Maybe we can look into it later as a lower-priority item. Do you think it would be difficult to resolve?

One thing I found:

Updating to this version on my local instance broke the add and edit post features for some reason, I get this when I try: https://i.imgur.com/4zI4qth.png

With the following error:

wperror.txt

Would you know why this could happen? I have not been able to reproduce this on another instance unfortunately, so it's hard to know what is happening here.

This was with the newest version of wordpress and no other plugin installed.

I've even reinstalled wordpress on my localhost, yet this error persist. On a ninja instance I've imported my local instance's charts, but I still cannot reproduce that error on another instance.

It only happens on this version, I can't reproduce the error using the live version of Visualizer.

Do you have an idea of what this error could be? I'm worried it could happen to other users' existing instances.

DanyCaissy commented 4 years ago

Note that this happens even after cleaning out the database. But it doesn't happen on ninja instances. My local instance was a 5.3 that got upgraded to 5.4 while ninja is 5.4 by default, maybe there is some sort of difference.

contactashish13 commented 4 years ago

@DanyCaissy this will still be a WIP for the next few days till more bugs like the stacked bug are uncovered and resolved. I still have to add the cypress tests so this PR should not be merged (it can still be tested as cypress is independent of the core code).

As for your error, I tried on 5.3 and did not face this. Maybe your upgrade was incorrect? Please test it on a raw 5.3 instance, a correctly upgraded WP and a raw 5.4 instance. Also can you share a video of where the error occurs so that I can follow the same steps?

DanyCaissy commented 4 years ago

@contactashish13

It seems to happen on a raw 5.3 instance now that I've tried.

2020-04-18_17-44-23.zip

The visualizer I've installed for reference:

visualizer.zip

contactashish13 commented 4 years ago

@DanyCaissy there seems to be something wrong with the way you are creating the build. Can you create a video of that process?

DanyCaissy commented 4 years ago

@contactashish13

2020-04-21_21-22-44.zip

This is the whole process, ignore anything past 4:20, my screen recorded bugged and I couldn't finish the video.

contactashish13 commented 4 years ago

Actually I develop directly off Git so maybe @rodica-andronache can verify if this is correct and if not, offer suggestions to fix this. I think you had a similar problem before as well on another PR. I believe both of you should be using the same methodology to test

DanyCaissy commented 4 years ago

@contactashish13 This is a process that a customer could be doing when installing Visualizer. Before asking for someone else to reproduce the issue, you should first try to reproduce it yourself, whether on this ticket or another. If you cannot reproduce it using the exact steps above then we can ask for Rodica to attempt it.

For the methodology, it's fine to test using different steps, as that can cover more possible ways a user might use/install the tool.

contactashish13 commented 4 years ago

A customer will not be using this method. They will be downloading from the repository.

We are not here to try out different methods for the sake of it. We should follow what the correct procedure is and Rodica should be able to help as she is has faced no problem. You can either take her help or you can work off GitHub desktop like I do.

On Apr 25, 2020, 00:55, at 00:55, Dany Caissy notifications@github.com wrote:

@contactashish13 This is a process that a customer could be doing when installing Visualizer. Before asking for someone else to reproduce the issue, you should first try to reproduce it yourself, whether on this ticket or another. If you cannot reproduce it using the exact steps above then we can ask for Rodica to attempt it.

For the methodology, it's fine to test using different steps, as that can cover more possible ways a user might use/install the tool.

-- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: https://github.com/Codeinwp/visualizer/issues/619#issuecomment-619198992

DanyCaissy commented 4 years ago

@contactashish13

If your issue is that I uploaded the zip instead of getting the plugin via git, I've replaced it with your version directly from git here:

2020-04-25_13-52-33.zip

The error prevails. As far as I know there shouldn't be a difference between both methods. If you have evidence of there being a difference let me know.

rodica-andronache commented 4 years ago

@contactashish13 I've also tried it on a WP 5.3.2 instance and got the same error

contactashish13 commented 4 years ago

@rodica-andronache I cannot reproduce this. I had a WP 5.2 instance that I upgraded to WP 5.3.2. I copied my entire plugin folder as is to that instance, activated the plugin (only plugin activated)

If you are unable to reproduce this fix, I would need a live 5.3.2 that shows this error.

DanyCaissy commented 4 years ago

@contactashish13 The videos above are reproducing it on a raw 5.3 instance, why have you asked for a video and haven't bothered trying it? I didn't go from WP 5.2 -> WP 5.3.2 in it.

I've reproduced it by downloading 5.3.2 now anyway, so both 5.3 and 5.3.2 have the issue.

Steps:

contactashish13 commented 4 years ago

@DanyCaissy you may want to read what I wrote here again about testing on a raw instance where I could not reproduce it. @rodica-andronache If either of you can, please share an instance where I can troubleshoot this.

Edit: By the way, cypress tests in Travis are run in WP 5.3 (I have only now changed them so that they work on the latest version of WP) and they have not failed. If the problem that you are stating is a common problem, those tests would fail too. Thus, I guess this is a local build/instance problem.

rodica-andronache commented 4 years ago

I couldn't create a jurassic instance to replicate this, I've tried multiple times and it worked ok. On my local indeed it happens, but not all instances, and I couldn't pinpoint it just for WP 5.3. In one case, I got the error on WP 5.4 for example. If possible, maybe the error message could be used to try to fix the error based on the code?

TypeError: this.activateMode is not a function at http://localhost:10015/wp-includes/js/media-views.min.js?ver=5.4.1:2:24708 at qt (http://localhost:10015/wp-content/plugins/visualizer/classes/Visualizer/Gutenberg/build/block.js?ver=3.4.3:16:5218) at Function.Uo (http://localhost:10015/wp-content/plugins/visualizer/classes/Visualizer/Gutenberg/build/block.js?ver=3.4.3:16:40372) at i._createModes (http://localhost:10015/wp-includes/js/media-views.min.js?ver=5.4.1:2:24668) at initialize (http://localhost:10015/wp-includes/js/media-views.min.js?ver=5.4.1:2:24077) at initialize (http://localhost:10015/wp-includes/js/media-views.min.js?ver=5.4.1:2:25732) at initialize (http://localhost:10015/wp-includes/js/media-views.min.js?ver=5.4.1:2:29623) at i.h.View (http://localhost:10015/wp-admin/load-scripts.php?c=0&load%5Bchunk_0%5D=hoverIntent,common,hoverintent-js,admin-bar,shortcode,backbone,wp-util,wp-backbone,media-models,wp-plupload,jquery-ui-widget,jqu&load%5Bchunk_1%5D=ery-ui-mouse,jquery-ui-sortable&ver=5.4.1:13:14225) at i.constructor (http://localhost:10015/wp-admin/load-scripts.php?c=0&load%5Bchunk_0%5D=hoverIntent,common,hoverintent-js,admin-bar,shortcode,backbone,wp-util,wp-backbone,media-models,wp-plupload,jquery-ui-widget,jqu&load%5Bchunk_1%5D=ery-ui-mouse,jquery-ui-sortable&ver=5.4.1:17:2608) at i.constructor (http://localhost:10015/wp-includes/js/media-views.min.js?ver=5.4.1:2:23524)

contactashish13 commented 4 years ago

@rodica-andronache I tried creating a few instances and it seems like this error can only be reproduced on an unlucky day :) If this is not a repeatable and consistent error, a solution may not be possible.

Looks like this has something to do with conflicts between loadash and underscore libraries: https://wpdevelopment.courses/articles/how-to-fix-activatemode-is-not-a-function-error-in-gutenberg/.

@HardeepAsrani can you shed some light on this?

contactashish13 commented 4 years ago

@HardeepAsrani can you please check and respond if you can help?

HardeepAsrani commented 4 years ago

Hey! Didn't see the last message. I'll take a look and let you know.

HardeepAsrani commented 4 years ago

Two things:

import {merge} from 'lodash';

with:

const { merge } = lodash;

That should solve it. Lodash is shipped with WP so you don't need to install it. @contactashish13

rodica-andronache commented 4 years ago

@contactashish13 didn't manage to test all options yet, but here's what I saw so far. While testing with the PRO version installed as well ( the current version, as I didn't see any PR related to this for the PRO version, besides the cypress tests )

contactashish13 commented 4 years ago

@rodica-andronache The second issue occurs for any chart in the master branch as well (I tried with Pie and Line).

TBH I'm not sure if gutenberg block has been tested completely as it has not gone through proper QA since development. I think we should focus on making that block error free before adding more features. I've also pointed out that there are many discrepancies between the block and the library in terms of the settings of each chart. We need to first list those and address them. @DanyCaissy or you can take this up and see if it a priority.

We may want to take a step back, fix everything in the block before testing this PR.

rodica-andronache commented 4 years ago

@contactashish13 ok, I'll first test the block with its existing features, and after everything is done there, we can move on with this one

vytisbulkevicius commented 1 year ago

+1 https://secure.helpscout.net/conversation/2051147506/342461/