search

d8-contrib-modules / addthis

Port AddThis to D8
GNU General Public License v2.0
3 stars 13 forks source link

Updated AddThisBasicToolbox to render proper markup. #33

Closed doylejd closed 9 years ago

doylejd commented 9 years ago
doylejd commented 9 years ago

@damontgomery Can you take a look at this real quick? This has the proper markup rendering, but there are a couple things that didn't work OOTB that I think should. They are listed in the TODO: section of the PR.

doylejd commented 9 years ago

@damontgomery This is ready to be looked at again.

damontgomery commented 9 years ago

@doylejd, I think for now this should be good.

I would recommend the switch statement logic be moved to a plugin system, but that could be done later.

I would recommend that the wrapper and element render elements be removed and to use a single element that is addthis_basic_toolbox and which takes a list of items. I'd put all the foreach logic into the twig template. These two element types are only used once and I can't see a use case for changing the markup elements (div / a / etc) unless it was being done site-wide in which case the pattern is to override the twig template. Feeding frontend config through the backend never works in my opinion.

doylejd commented 9 years ago

@damontgomery Thanks for the feedback - your suggestions have been moved into the issue queue. Closing this out.