Support from Acquia helps fund testing for Drupal Acquia logo

Comments

medienverbinder’s picture

Version: » 7.x-1.x-dev

Or maybe an option for the configuration backend to select the css file ?

criz’s picture

Yes, maybe we should detect if the fontawesome library is installed. When this is not the case an option can be provided to load the css file including fontawesome. But you're right, as it possible to load the fontawesome css also in a theme or somewhere, a checkbox would be good to disable this.

medienverbinder’s picture

Status: Active » Needs review
FileSize
37.75 KB
2.94 KB

Here's a suggestion for a solution:

With an additional option in configuration backend you can select the ccs file as a library variant.
Depending on the setting the following files are loaded via the library api:

Variant 'complete':
=> build/shariff.complete.css
=> build/shariff.complete.js
Variant 'min':
build/shariff.min.css
build/shariff.min.js

Loading library variant:

// Load shariff library.
$variant = variable_get('shariff_css', 'min');
libraries_load('shariff', $variant);
criz’s picture

Sounds good! Only that I wouldn't use shariff.complete.js, as it contains jQuery, that is always present in Drupal 7.

  • build/shariff.complete.js contains all dependencies
  • use build/shariff.min.js, if jQuery is already included in your site
medienverbinder’s picture

Yes....You're right! (some typos)

Variant 'complete':
=> build/shariff.complete.css
=> build/shariff.min.js

and

$form['shariff_css'] = array(
    '#title' => t('CSS'),
    '#description' => t('Please choose a css variant.'),
    '#type' => 'radios',
    '#options' => array(
      'min' => t('Min. (shariff.min.css => If Font Awesome is already included in your site)'),
      'complete' => t('Complete (shariff.complete.css => Contains all dependencies)'),
    ),
    '#default_value' => $settings['shariff_css'],
  );
medienverbinder’s picture

Here is a patch for the latest dev version ("data-twitter-via" commit included => there was an overlap)

medienverbinder’s picture

medienverbinder’s picture

I think it makes no sense, to take the variable for the css file in the settings array?
Here's another change.

criz’s picture

Awesome, works as intended.

What do you think about having a third option to not include any css? Would be good to have for people doing their own button styles.
I also improved the help text and the options a little bit, hope you like it.

medienverbinder’s picture

Great idea! I also have another suggestion for a fourth variant "custom": The "shariff.min.js" file and a custom css file within the module? (example: shariff/css/shariff_custom.css)

What do you think about the term "none" instead of "naked" as an option for the variant without css?
The loading of libraries I've changed to a switch statement.

criz’s picture

Yes, today I also was thinking about having a custom css file distributed in the module. But as long we have no custom css file we shouldn't provide this option.

The term "none" is semantically correct as an option for the CSS settings field, but it's not really a good name for the library variant. As you introduced the switch for loading the variant maybe we can name the variant "naked" and the option in the css settings field "none"?

And did you test how the buttons work without having saved the settings form once?
Maybe we should add a fallback, in the case when the variable is not present:

<?php
  $variant = variable_get('shariff_css', 'complete');
?>
medienverbinder’s picture

The css file option "custom" is for your own css definitions. I thought it would be better if the custom css for the shariff module remains within the module and not in the theme?

You're right, I'll rename the library variant as you suggested.

I have tested all the options and the default value for the css file already included in: shariff_settings_form()

 '#default_value' => variable_get('shariff_css', 'min'),

The new patch comes later ... unless you're faster;-)

criz’s picture

Hmm, I don't think that custom code or css should live in contrib modules. Great that the default values work!

medienverbinder’s picture

You're right with the custom css file ... that makes no sense. I have modified the README.txt and changed the option text for the variant selection "naked" to "none". I think this patch is quite good and hope you like it too!

criz’s picture

Thanks! I added a line in the readme and changed a little bit the default loading. We need to provide a default value for the variant because when installed freshly and the settings form hasn't been saved the shariff_css variable wouldn't be set. Also I think that loading the complete css would be the better default option, as the icons would work in every case then. But good job!

If you are okay with these changes (check latest dev snapshot) I would release version 1.1.

medienverbinder’s picture

I am very pleased with the result of our work. I look forward to the new version. Many Thanks!

criz’s picture

Status: Needs review » Fixed

:)

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.