Skip to content

Conversation

@DibyaGoswami
Copy link

What does this Pull Request do?

It lets the state of the "Text Overlay" checkbox in the mirador settings, affect whether or not the text overlay is enabled.

  • Related GitHub Issue: (link)

This is the link to the issue on Github:
#59

What's new?

An in-depth description of the changes made by this PR. Technical details and
possible side effects.

  • Changed TextOverlay.php - plugins now check their own checkbox state and provide enabled/disabled configuration accordingly
  • Changed islandora_miradora.module - removed conditional plugin loading, now always calls all plugins (they decide internally what config to provide)
  • Added cache invalidation tag so configuration changes take effect immediately without manual cache clearing
  • This does not add any dependencies
  • This will not affect existing code outside of the presence of the text overlay being dependent on the state of the "Text Overlay" checkbox in the mirador settings, in configuration

How should this be tested?

A description of what steps someone could take to:

  • Before applying the changes, to reproduce the issue, you can uncheck the "Text Overlay" checkbox and see if the text overlay is present, and then check the "Text Overlay" checkbox and then observe the text overlay's presence, and you will find (from our testing) that the text overlay's presence is not affected by the state of the checkbox, and is tied to the hardcoded code in TextOverlay.php
  • Similar to trying to reproduce the issue, apply the changes, then uncheck the "Text Overlay" checkbox, save the configuration, and then check if the text overlay is absent (it should be absent), and then check the "Text Overlay" checkbox, save the configuration and then check if the text overlay is present (it should be present).

Documentation Status

  • This changes fix the state of the site and code so that it works as intended, as before it was not working how it was supposed to.

Additional Notes:

None

Interested parties

@digitalutsc

…ay checkbox in the mirador settings not affecting the presence of the text overlay
Comment on lines 58 to 64
// if (isset($enabled_plugins[$plugin_id])) {
// $plugin_instance = $mirador_plugin_manager->createInstance($plugin_id);
// /**
// * @var Drupal\islandora_mirador\IslandoraMiradorPluginInterface
// */
// $plugin_instance->windowConfigAlter($window_config);
// }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be removed if it's no longer used

Suggested change
// if (isset($enabled_plugins[$plugin_id])) {
// $plugin_instance = $mirador_plugin_manager->createInstance($plugin_id);
// /**
// * @var Drupal\islandora_mirador\IslandoraMiradorPluginInterface
// */
// $plugin_instance->windowConfigAlter($window_config);
// }

Comment on lines 66 to 71
// Always call plugins - they will check their own state
$plugin_instance = $mirador_plugin_manager->createInstance($plugin_id);
/**
* @var Drupal\islandora_mirador\IslandoraMiradorPluginInterface
*/
$plugin_instance->windowConfigAlter($window_config);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The type hint should go when the variable is instantiated

Suggested change
// Always call plugins - they will check their own state
$plugin_instance = $mirador_plugin_manager->createInstance($plugin_id);
/**
* @var Drupal\islandora_mirador\IslandoraMiradorPluginInterface
*/
$plugin_instance->windowConfigAlter($window_config);
/**
* @var Drupal\islandora_mirador\IslandoraMiradorPluginInterface
*/
$plugin_instance = $mirador_plugin_manager->createInstance($plugin_id);
$plugin_instance->windowConfigAlter($window_config);

@DibyaGoswami
Copy link
Author

The code has been edited based on the suggestions given

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants