Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OPENEUROPA-1747: Add social sharing block. #46

Merged
merged 5 commits into from
Apr 2, 2019
Merged

Conversation

imanoleguskiza
Copy link
Member

OPENEUROPA-1747

Description

Add social sharing block.

Change log

  • Added: Social sharing block
  • Changed:
  • Deprecated:
  • Removed:
  • Fixed:
  • Security:

Commands

[Insert commands here]

'system',
'user',
'oe_webtools_social_share',

Copy link
Contributor

Choose a reason for hiding this comment

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

Remove empty line please

/**
* {@inheritdoc}
*/
public static $modules = [
Copy link
Contributor

Choose a reason for hiding this comment

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

The parent of this is protected so this one should be as well.

$render = $this->buildBlock('social_share', $config);
$html = (string) $this->container->get('renderer')->renderRoot($render);
$crawler = new Crawler($html);
// Make sure that search form block is present.
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not a search form.

/** @var \Drupal\Core\Block\BlockBase $plugin */
$plugin = $this->container->get('plugin.manager.block')->createInstance($block_id, $config);
// Inject runtime contexts.
if ($plugin instanceof ContextAwarePluginInterface) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't need any of the contexts to be injected as we dont use contexts in this block. So this entire method is overkill, you can just get the plugin directly in testSearchBlockRendering()

* category = @Translation("Webtools"),
* )
*/
class SocialShare extends BlockBase {
Copy link
Contributor

Choose a reason for hiding this comment

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

Blocks typically have their name suffixed with Block: SocialShareBlock

],
'stats' => TRUE,
];
return [
Copy link
Contributor

Choose a reason for hiding this comment

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

The only reason why this widget is showing up when placing the block is because the other webtools submodules are enabled. Each widget needs to also attach the smart loaders library. Check for example oe_webtools_laco_widget_page_attachments() and attach it also here.

brummbar
brummbar previously approved these changes Mar 29, 2019
* Implements hook_page_attachments().
*/
function oe_webtools_social_share_attachments(array &$attachments) {
$attachments['#attached']['library'][] = 'oe_webtools/drupal.webtools-smartloader';
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's move this to the block render array itself, so that it is included only when the block is rendered.

runner.yml.dist Outdated
@@ -24,6 +24,7 @@ drupal:
- "./vendor/bin/drush en oe_webtools oe_webtools_laco_service -y"
- "./vendor/bin/drush en oe_webtools oe_webtools_laco_widget -y"
- "./vendor/bin/drush en oe_webtools oe_webtools_cookie_consent -y"
- "./vendor/bin/drush en oe_webtools oe_webtools_social_share -y"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- "./vendor/bin/drush en oe_webtools oe_webtools_social_share -y"
- "./vendor/bin/drush en oe_webtools_social_share -y"

It should be fixed in all lines except the first 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed. Let's put all our modules in one line

@brummbar
Copy link
Contributor

Actually my review should have been of "Request changes" type but i misclicked.

@upchuk upchuk merged commit aeff677 into master Apr 2, 2019
@upchuk upchuk deleted the OPENEUROPA-1747 branch April 2, 2019 06:22
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.

3 participants