nikathone → made their first commit to this issue’s fork.
A small nitpick.
nikathone → made their first commit to this issue’s fork.
nikathone → created an issue.
andileco → credited nikathone → .
This is ready for review. To review it you should remember to add the entity and enable it. Not sure if we need to implement an update hook to install the entity module on existing sites.
nikathone → created an issue.
Looks good to me. Thanks for the help with the issue description update and the code to that use the new API.
nikathone → created an issue.
nikathone → made their first commit to this issue’s fork.
nikathone → made their first commit to this issue’s fork.
nikathone → made their first commit to this issue’s fork.
@bluegeek9 we did that to make the config form backwards compatible in case another user/module have extended the module before we add those dependencies.
In the future major version upgrade of chart we will remove the ternary operators.
nikathone → created an issue.
nikathone → created an issue.
nikathone → made their first commit to this issue’s fork.
There is a similar issue in the main gin 🐛 Edge case: $settings->get('enable_darkmode') can cause WSOD Active theme, where instead of using "false" they are using "unknown" I think we should try to set it to the same value.
Adding related issue from gin toolbar not sure if they should solve the same way.
Looks good to me.
Ok. Let get rid of the trait.
andileco → credited nikathone → .
andileco → credited nikathone → .
nikathone → made their first commit to this issue’s fork.
nikathone → made their first commit to this issue’s fork.
Hi @deasly, I didn't add a setting but now the column key should be present. You should have something like
<?php
["q1___value_13" => "43128", "Q1" => "43128"]
?>Note that if you apply aggregation and use a field twice then it might worth get the value from the alias key instead of the column based your need. Sorry for the disruption caused by the alias addition.
nikathone → made their first commit to this issue’s fork.
nikathone → made their first commit to this issue’s fork.
nikathone → made their first commit to this issue’s fork.
nikathone → made their first commit to this issue’s fork.
andileco → credited nikathone → .
nikathone → created an issue.
nikathone → created an issue.
nikathone → created an issue.
I would say let focus on the reported error like Missing cache backend declaration for performance. and \Drupal calls should be avoided in classes, use dependency injection instead if they are not breaking backward compatibility.
nikathone → made their first commit to this issue’s fork.
Not sure if I am missing something but the fork doesn't contain new codes. I also switched the version to latest chart version and I think this means that the fork might need to be rebased.
I looked at the code and for me this feels like a limited use case. There is a way this can be accomplished from a custom module:
- Implement a hook_library_info_alter and swap the
charts/modules/charts_highchartsjs/charts_highcharts.jswith a custom js file - Or add a custom js file that override the chart configurations using javascript. Please see the example of the library declaration at https://git.drupalcode.org/project/charts/-/blob/5.1.x/modules/charts_ap... and the javascript file overriding the config before highcharts load them can be found at https://git.drupalcode.org/project/charts/-/blob/5.1.x/modules/charts_ap.... Note the javascript weight in the library file.
Please let us know if this approach doesn't work and we can try to see if the charts module can be updated to include more method to alter the config file.
@jrochate can you open two separate feature request issues for point 1 and 2?
@rex.barkdoll can you try to update to the latest charts version and let us know if you still experiencing this issue?
nikathone → created an issue.
nikathone → made their first commit to this issue’s fork.
Added related issues that will help with the code added here.
We do have $chart['#view']. The users can get the view ID from it. We might also need to document this at https://git.drupalcode.org/project/charts/-/blob/5.1.x/charts.api.php?re....
Looks good for me.
nikathone → created an issue.
nikathone → created an issue.
andileco → credited nikathone → .
nikathone → changed the visibility of the branch 3457776-fix-triggers to hidden.
Added the test for views using the filter. This is ready for review.
nikathone → created an issue.
Merge the code to dev.
Marking this fix.
nikathone → created an issue.
Sorting the options ASC without adding any configuration for the user. I think should be kind of expected. @ultimike please let us know if you still think the option to let the user opt in into sorting is needed.
nikathone → made their first commit to this issue’s fork.
andileco → credited nikathone → .
andileco → credited nikathone → .
nikathone → created an issue.
andileco → credited nikathone → .
andileco → credited nikathone → .
nikathone → created an issue.
Ended up adding more codes that are not related to the issue but I think they are going to help us in the long run especially with the introduction of some test coverage for the main Select.php class.
Some how the MR is mergeable but it contains a commit that I didn't make. Adding a patch file if someone need it for composer patches.
nikathone → made their first commit to this issue’s fork.
Looks good to me.
andileco → credited nikathone → .
andileco → credited nikathone → .
andileco → credited nikathone → .