I think it'd be better if the CSS styles (for widgets, in particular) were in one file instead of two. Drupal adds a js class to the <html> element to allow for easier JavaScript-specific CSS targeting. Not to mention it's rather unsightly (not to mention unnecessary) to have document.write in your markup. ;)

I've included a patch that solves this issue. I've also modified the extra_voting_forms_insert_js_css() function and made it a bit more elegant too. (While we're on the subject, you might want to consider renaming that function to something more accurate (since it no longer inserts CSS) — I didn't because it'd probably break a lot of existing sites if I did.)

Thanks.

CommentFileSizeAuthor
#5 combine_js_css_with_main_css2.patch36.47 KBgabriel.
#1 combine_js_css_with_main_css1.patch33.68 KBgabriel.
combine_js_css_with_main_css.patch18.98 KBgabriel.

Comments

gabriel.’s picture

gabriel. commented
StatusFileSize
new combine_js_css_with_main_css1.patch33.68 KB

Oops!

This new patch removes the old, unnecessary files. Much better.

mercmobily’s picture

mercmobily commented
Status: Active » Needs review

Hi,

I don't think I will ever thank you enough for this patch. Finally, the javascript in extra_voting_forms done right!
I am very inept in terms of Javascript and CSS. You probably noticed by reading the code. So, I _beg_ you not to abandon this issue, and bare with me for a minute :-D

Some important -- very important -- questions:

* Does this break the CSS of the _many_ existing sites out there? If so, is there a way to make things "compatible"?
* This is an IMPORTANT and drastic change. We have 3 widget sets. Did you really fix the lot?
* Did you thoroughly test this? I am asking because the number of people who will flood the forums if something is broken is impressive :-D
* Is there _any_ way for you... err... actually check that this "the" way you're meant to pass variables to Javascript in Drupal? I am asking because _surely_ there should be a better way of doing it. I *seem* to remember that there was a "neater" way to do it, so that variables ended up in the Drupal.* domain in Javascript. Am I remembering this wrong?

Again, thank you so much... I was hoping somebody would submit precisely something like this...

Merc,

gabriel.’s picture

gabriel. commented

  1. * Does this break the CSS of the _many_ existing sites out there? If so, is there a way to make things "compatible"?

    It shouldn't break anything, unless they manually edited the CSS files directly, in which case the usual rules apply (they'll have to reapply any changes they originally made, etc.).

  2. * This is an IMPORTANT and drastic change. We have 3 widget sets. Did you really fix the lot?

    For each of the widgets, what I did was append the contents of extra_voting_forms_js.css to extra_voting_forms_all.css, and then prefix each of those CSS declarations with html.js . That should be all there is to it, and shouldn't cause any odd rendering issues (that weren't there before) since the style rules are getting loaded in the same order they were before.

  3. * Did you thoroughly test this? I am asking because the number of people who will flood the forums if something is broken is impressive :-D

    Not extremely thoroughly, but like I said before, the order the CSS rules are getting loaded in is still the same, so it shouldn't change anything visually. The only thing I can think of that this might impact would be you'd have to update your CSS since the new have a slightly higher specificity than how it was before, which would be as easy as prefixing html.js to their respective declaration. However I don't think many people would need to worry about this, since they'd edit the widget styles instead, and 95% of the time things can just be left the way they are without issue.

  4. * Is there _any_ way for you... err... actually check that this "the" way you're meant to pass variables to Javascript in Drupal? I am asking because _surely_ there should be a better way of doing it. I *seem* to remember that there was a "neater" way to do it, so that variables ended up in the Drupal.* domain in Javascript. Am I remembering this wrong?

    To be honest, I wasn't really focusing on that—this was just something that I happened to come across since it was in the same block of code I was working with. But now that you mention it, there is a way to add your JavaScript variables into the Drupal.* namespace. I noticed jstools.module does this, here's the code they use:

    <?php
drupal_add_js(array(
  'jstools' => array(
    'cleanurls' => (bool) variable_get('clean_url', '0'),
    'basePath' => base_path(),
  ),
), 'setting');
?>

    Which gets parsed into:

    <script type="text/javascript">Drupal.extend({ settings: { "jstools": { "cleanurls": true, "basePath": "/" } } });</script>

    And that's definitely is a lot cleaner, especially when you've got multiple modules calling this, they all get grouped into one <script> element so you don't have multiple unnecessary <script>s in your <head> (at least I think that's what happens from what I've seen).

mercmobily’s picture

mercmobily commented

Hi,

Is there any way for you to:

1) Test that rendering doesn't break in IE6, IE7 and Firefox with the new CSS

2) Change the variable assigning to jstools (which implies changing a bit of the Javascript as well

...? If not, that's fine -- I will try and do it myself. But if you could, I will build a big virtual statue for you! :-D

(Please note that I am asking this as a _favour_... I have 6 issues to deal with right now, and really don't want this one in particular to end up in a queue...)

Merc.

gabriel.’s picture

gabriel. commented
StatusFileSize
new combine_js_css_with_main_css2.patch36.47 KB

Ok, here we are.

If the widget CSS styles weren't overridden, then everything should render just fine, and even if they were (i.e. if the changes were made outside of the widget's stylesheet) then there's a 98% chance it'll still work fine, since the theme's stylesheet gets loaded last and hence would take precedence when defining styles.

And I added the JS variables into the Drupal.settings.* namespace and renamed the variables where appropriate.

new stuff:

  • I renamed extra_voting_forms_insert_js_css() to extra_voting_forms_init() (and created an empty function with the old name to maintain backwards compatibility). This way users do longer need to modify their page.tpl.php to add the necessary javascript to the <head/> (old pages with the function call will still work properly, but shouldn't need it anymore).
  • I also removed extra_voting_forms_js_variables() since it wasn't being used anywhere and appeared to be unnecessary code duplication.
mercmobily’s picture

mercmobily commented
Status: Needs review » Fixed

Hi,

THANK YOU...
I am glad this patch came early enough in the game!!!

Applied, all themes tested.

Thanks,

Merc.

Anonymous’s picture

Anonymous (not verified) commented
Status: Fixed » Closed (fixed)

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