Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
See https://github.com/heiseonline/shariff/commit/577441743b74a3846ddb190c87....
shariff.min.css does not any longer include Font Awesome webfont
reference. Use shariff.complete.css, if you need every CSS dependency
in one CSS file.
As we should stick to shariff.min.css I guess we need to add Font Awesome as a dependency.
Comments
Comment #1
medienverbinder CreditAttribution: medienverbinder commentedOr maybe an option for the configuration backend to select the css file ?
Comment #2
crizYes, 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.
Comment #3
medienverbinder CreditAttribution: medienverbinder commentedHere'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:
Loading library variant:
Comment #4
crizSounds good! Only that I wouldn't use shariff.complete.js, as it contains jQuery, that is always present in Drupal 7.
Comment #5
medienverbinder CreditAttribution: medienverbinder commentedYes....You're right! (some typos)
and
Comment #6
medienverbinder CreditAttribution: medienverbinder commentedHere is a patch for the latest dev version ("data-twitter-via" commit included => there was an overlap)
Comment #7
medienverbinder CreditAttribution: medienverbinder commentedComment #8
medienverbinder CreditAttribution: medienverbinder commentedI think it makes no sense, to take the variable for the css file in the settings array?
Here's another change.
Comment #9
crizAwesome, 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.
Comment #10
medienverbinder CreditAttribution: medienverbinder commentedGreat 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.
Comment #11
crizYes, 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:
Comment #12
medienverbinder CreditAttribution: medienverbinder commentedThe 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()
The new patch comes later ... unless you're faster;-)
Comment #13
crizHmm, I don't think that custom code or css should live in contrib modules. Great that the default values work!
Comment #14
medienverbinder CreditAttribution: medienverbinder commentedYou'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!
Comment #16
crizThanks! 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.
Comment #17
medienverbinder CreditAttribution: medienverbinder commentedI am very pleased with the result of our work. I look forward to the new version. Many Thanks!
Comment #18
criz:)