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

Abstract out Block and Field Formatters into submodules. #12

Merged
merged 5 commits into from
Aug 12, 2015

Conversation

doylejd
Copy link
Contributor

@doylejd doylejd commented Aug 12, 2015

  • Abstracted out blocks and field formatters into their own modules.
  • Added configuration settings to block to match AddThisBasicToolbox for more flexibility.
  • Remove global addition of addthis_widget. Place it into render functions of fields/blocks.

…ded configuration settings to block to match AddThisBasicToolbox for more flexibility.
@@ -24,8 +24,8 @@ function addthis_page_attachments(array &$page){

// Include the widget js when we include on all but admin pages.
//if (!path_is_admin(current_path()) && AddThis::getInstance()->getWidgetJsInclude() == AddThis::WIDGET_JS_INCLUDE_PAGE) {
$manager = \Drupal\addthis\Services\AddThisScriptManager::getInstance();
$manager->attachJsToElement($page);
//$manager = \Drupal\addthis\Services\AddThisScriptManager::getInstance();
Copy link
Contributor

Choose a reason for hiding this comment

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

I would remove this commented out code, it's versioned

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nerdstein Should be updated in this PR.

$form['settings']['addthis_settings']['type'] = array(
$form['settings']['addthis_settings']['share_services'] = array(
'#title' => t('Services'),
'#type' => 'textfield',
Copy link
Contributor

Choose a reason for hiding this comment

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

This is going to be pulled in as a checklist in a future PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nerdstein I want to get some more feedback on this since the interface for the checkboxes is so large. Providing a link to the options and textfield seems usable, but I agree that it is probably not the best approach.

I want to keep mobile editing in mind here - a list of 300 checkboxes might provide more problems.

Copy link
Contributor

Choose a reason for hiding this comment

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

Might be a good first step and add an issue to improve the UX

@nerdstein
Copy link
Contributor

This looks pretty good. i'd groom the readme just a bit more and i'll merge

@doylejd
Copy link
Contributor Author

doylejd commented Aug 12, 2015

@nerdstein README updated.

nerdstein added a commit that referenced this pull request Aug 12, 2015
Abstract out Block and Field Formatters into submodules.
@nerdstein nerdstein merged commit bb4a4bb into d8-contrib-modules:master Aug 12, 2015
@nerdstein
Copy link
Contributor

Merged

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