Call plugin callbacks in the order they are configured #1285
Conversation
…issue1148-order-of-plugin-rendering
Codecov Report
@@ Coverage Diff @@
## master #1285 +/- ##
============================================
+ Coverage 69.33% 70.32% +0.98%
- Complexity 1648 1685 +37
============================================
Files 32 32
Lines 4047 4195 +148
============================================
+ Hits 2806 2950 +144
- Misses 1241 1245 +4
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Can you please use the PR template to describe the changes in this PR?
Also, it would be helpful to provide a minimal example of how to test this. For example a test configuration with 2-3 plugins that were in the wrong order before but are correctly ordered with the changes in this PR.
There was a problem hiding this comment.
The PR introduction is now good. Here are some suggestions for changes:
- if it's easy, make the tests cover the case of non-null $names parameter for getPluginCallbacks (see suggestions in comments)
- the
<script>elements for plugins in HTML code should be in the configuration order of plugins - there should be
concept-info-beforeandconcept-info-afterelements inconcept-shared.twig, like this:
Around line 15:
<div id="concept-info-before"></div>
<div class="concept-info{% if concept.deprecated %} deprecated-concept{% endif %}">
Around line 211:
</div>
</div>
<div id="concept-info-after"></div>
{% endfor %}
| * @return array | ||
| */ | ||
| public function getPluginCallbacks($names=null) { | ||
| if ($names) { |
There was a problem hiding this comment.
The tests don't cover the case of $names being passed. I think it should be easy to modify the test for getPluginCallbacks. I will suggest below...
tests/PluginRegisterTest.php
Outdated
| { | ||
| $plugins = new PluginRegister(); | ||
| $this->assertEquals(array('test-plugin' => array('plugins/test-plugin/stylesheet.css')), $this->mockpr->getPluginsCSS(array('test-plugin'))); | ||
| $this->assertEquals($this->mockpr->getPluginCallbacks()['test-plugin1'], |
There was a problem hiding this comment.
Here passing a $names parameter to getPluginCallbacks would increase test coverage.
|
Kudos, SonarCloud Quality Gate passed!
|
|
The Javascript execution order is now good. The new I added some annotations to increase test coverage. The case of non-empty The documentation for plugins still needs to be updated. I think we need updated guidelines on how plugins should insert their elements into the DOM structure, namely:
This way the order of the inserted elements remains the same as the configured order of plugins (which is the order for the function calls to the plugin callbacks). It doesn't matter how long async operations take, because the order has been established by the placeholder elements and won't change even if they are updated. I will merge this PR as it does its job; updating the documentation for plugins still remains as a task. |
osma
changed the title
Reasons for creating this PR
Fixes reopened issue #1148. Orders also callback names of plugins in the same order as sorted in VocabularyConfig.php and configured in skosmos:vocabularyPlugins.
Link to relevant issue(s), if any
Description of the changes in this PR
Plugin order is configured with skosmos:vocabularyPlugins. PR #1201 made it possible to define order of plugins in configuration. PluginRegister class reorders the plugins in alphabetical order of plugin file names, but this fix rearranges the callback names to the same order as in skosmos:vocabularyPlugins. PR also adds the possibility to define parameter plugins outside of the ordered plugin list
Known problems or uncertainties in this PR
Plugin rendering order is reverse than the order configured in skosmos:vocabularyPlugins. To solve this, Skosmos HTML template could have additional empty div element (e.g.
<div id="concept-info-after"></div>), which serves as a location for widgets. Each widget could then reserve own empty div element during callback with JQuery$('#concept-info-after').append(...). Then the rendering would be done in same order as in skosmos:vocabularyPlugins.In same way a similar div element (e.g.
<div id="concept-info-before">) could be added for widgets with message (e.g. https://github.com/NatLibFi/Skosmos-widget-message) above the concept info.