d8-contrib-modules / addthis Goto Github PK

Abstract out the BasicButton and BasicToolbox settings forms so that they can be reused in the Field & Block implementations.

@todo:

• Abstract out form's into AddThis.php class.
• Update block to allow for BasicButton and BasicToolbox
• Test to ensure settings work properly across plugin types.

Hello,

I've try this module for drupal 8 (8.1.8) and when I create a addthis field I've go this fatal error :
Fatal error: Declaration of Drupal\addthis\Plugin\Field\FieldFormatter\AddThisFormatter::viewElements() must be compatible with Drupal\Core\Field\FormatterInterface::viewElements(Drupal\Core\Field\FieldItemListInterface $items, $langcode) in /modules/addthis/src/Plugin/Field/FieldFormatter/AddThisFormatter.php on line 39

There are several form/styling issues on the AddThis Admin page. (admin/config/user-interface/addthis)

@todo:

• Fieldsets should be collapsable.
• Enabled Services & Compact Menu Enabled Services labels are overlapping

• Implement the bubble counter attributes (See D7 addthis_displays.addthis.inc for D7 implementation)
• Should add these conditional attributes to the D8 render element preprocess functions so that they can render in the proper tags printed from the toolbox.

Currently we are using a helper method in the AddThis.php class to provide the form elements for the AddThisBasicButton and AddThisBasicToolbox config options which are shared between formatter and block.

After talking to Kris yesterday, he suggested we take a look at a partial form or traits. He mentioned the form could be problematic because of how it gets pulled in and the fact we only need partial form elements that will be shared.

• Keep in mind how we make this extendable. Do we want to provide other modules the ability to add renderElement types that can be picked up by our block or do we just provide what we have and if they want something new, they can create both elements. (And still use the addThis for global config/js integration. How much benefit does this give?)

• Need to get some test coverage into AddThis.

After adding addthis field to a content type and try to visit form display I get following error:

PHP Fatal error: Declaration of Drupal\\addthis\\Plugin\\Field\\FieldFormatter\\AddThisBasicButtonFormatter::viewElements() must be compatible with Drupal\\Core\\Field\\FormatterInterface::viewElements(Drupal\\Core\\Field\\FieldItemListInterface $items, $langcode) in /modules/contrib/addthis/src/Plugin/Field/FieldFormatter/AddThisBasicButtonFormatter.php on line 26

Fix:

• Add $langcode parameter to viewElements() method of 3 FieldFormatters that are implemented in this module.

The Google Analytics integration with the AddThis module was modified to D8 constructs, but the integration has not been fully tested with the D8 Google Analytics module (currently only a -dev branch).

This needs to be tested and the implementation improved as needed to support this functionality.

Abstract out the Field Formatters for AddThisBasicButtonFormatter.php and AddThisBasicToolboxFormatter.php into the addthis_displays module.

We want to keep the basic module setup as:

• addthis - The basic API integration to interface with AddThis. Provides no rendering for AddThis elements.
• addthis_fields - Provides field formatters to allow rendering AddThis in content types.
• addthis_block - Provides configurable AddThis block plugin to be used.

In the Drupal 7 version of AddThis the block can be configured based on any FieldFormatter that is defined for AddThis. With the changes in D8, I was not able to reproduce this functionality.

@todo:

• Investigate the best way to make the block extensible by future contrib/custom modules.
• Can we use the FieldFormatters in the same way that D7 did?
• Is there a better option to handle the Field/Block displays to make them extensible by external modules? (Ie. Controllers, ??)

Revise the way the global AddThis libraries are added to the page. Using hook_page_attachments() will break the aggregation. Investigate only loading this library when we are rendering an AddThis element to the page.

The hover menu on the "Share" button is currently broken. It is displaying in the middle of the screen instead of relative to the button. The formatting is broken as well.

Fix the rendering of AddThisBasicToolbox in the block and field to match the HTML in the D7 module. Ensure that it respects configuration settings and prints the proper markup.

TODO in modules/addthis/src/Services/AddThisScriptManager.php
private function getJsAddThisConfig() { }

Per @damontgomery's comment on our last PR:

So, the recommended way to access the language setting is to get a language handler service using dependency injection. The use is actually shown here, https://www.drupal.org/node/2133171

I think you'd actually want to inject Drupal\Core\Language\LanguageManager using language_manager as the service name or @language_manager as the argument to this service. Then you can assign this to $this->languageManager and get the language with the method $this->languageManager->getCurrentLanguage()

The AddThis class was created as a helper class in the D7 version of the module. This class has been significantly down-sized with the port to D8 since the new OO design allowed for better abstraction.

@todo:

• Determine if the AddThis class is right to be converted into a Controller
• Determine if pieces of the AddThis class can be abstracted out to a better home.
• Deprecate (remove) AddThis.php if possible

The Smart Layers API allows you to have more control over how AddThis functions on your website. For more information see: http://support.addthis.com/customer/portal/articles/1200473-smart-layers-api

Initial thoughts on how to do this and to discuss further with @nerdstein and @damontgomery :

• Create a configuration entity to handle the config?
• Allow the configuration to be attached to a route? Maybe set a default and allow specific ones to be overridden? My thought is that you will want most of your site to be consistent - but if you have promo pages you might want to allow for alternate options.
• Could do this at the block level, but that doesn't feel right to me.

Option 2:

• Provide a new global configuration page for site-wide settings for SmartLayersAPI.
• Add the SmartLayersAPI config to the AddThisScriptManager and update the JS as needed

The validation function used for the BasicToolbox form is currently in addthis.module. Since this is not a hook, it should not live in the .module file.

@todo:

• Find a better place to keep the validation function. Investigate a better place to put this validation function. The form is located in the AddThis class, but I was unable to get the validation function to get picked up there. Could be a simple syntax issue.

Revist the field implementations to use the new paradigm that was defined in the AddThisBlock. Instead of calling AddThis:: class to get markup, revise to take advantage of the renderElements that were created.

The Advanced AddThis Settings page is not accessible through the configuration menu.

• URL can be accessed here /admin/config/user-interface/addthis/advanced

@todo:

• Fix the menu routing to allow users to access the advanced AddThis settings. Would be ideal if this was a tabbed interface for Basic/Advanced settings.

D7 version of AddThis implemented a block that allowed users to chose a field formatter to render it. The block used the default field settings to control the rendering.

D8 implements blocks as plugins which gives more flexibility in its configuration. It would allow for more flexibility if we allow the block to be fully configurable instead of depending on a field formatter for the configuration and rendering.

@todo

• Expand the block configuration to allow for additional configuration.
• Update render method (Currently always renders addthis_basic_toolbox using the field formatter default settings)

Permissions for menu paths are currently set to "administer site configuration". This should be updated to a custom addthis permission.

@todo:

• Add new permission "administer addthis settings" and apply to routing.
• Add new permission "administer advanced addthis settings" and apply to routing.

There are currently no source, process, or destination plugins making it difficult to move existing data from d7 to d8 for this module.

The rendering of the AddThis elements does not look good OOTB. Need to investigate to see if this is caused by the RenderElement markup, Additions needed to the Twig Templates or some custom CSS that needs to be added and loaded.

D8 provides a robust URL class as part of core. Look at replacing AddThisWidgetJsURL.php class with the core URL class.

• Implement SimpleTest tests for the module.

• Remove the RenderElement code and twig templates for AddThisElement and AddThisWrapper.
• Update the AddThisBlock class to setup the data to pass to rendering. Most of the logic can be handled in a twig template rather than in the build() function.
• Update the AddThisBasicToolbox and AddThisBasicButton Render Elements to setup the variables to pass to Twig templates.
• Update the twig templates to handle the logic that the build() function used to do.
• Refine documentation in PreRender/Twig templates per implementation

Driving Factors: All display logic should be handled in the theme layer.