Hi,

I have an small contribution, which helps to select the weight of the js or css added. How could I send it to you? I am used to git but not too much to patch, is it better if I make a patch instead of contributing via git?

Thank you.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

joelstein’s picture

You can upload a patch; that will do. Thanks!

alexmoreno’s picture

Version: 7.x-1.4 » 7.x-1.x-dev
Category: task » feature
Status: Needs review » Patch (to be ported)
FileSize
2.97 KB

Here is the patch, let me know if everything is ok if someone try it :-).

DamienMcKenna’s picture

Status: Patch (to be ported) » Needs review

Corrected the issue status.

DamienMcKenna’s picture

Title: Contribution: weight » Add a selector to control the weight

Clarified the issue title.

DamienMcKenna’s picture

Status: Needs review » Needs work

This needs a little work:

  • It might be better to give a list of numbers to choose from rather than use a text field.
  • The per-type files should still have their weight one less than the per-node file.
  • The code in cpn_settings_validate doesn't match the Drupal coding standards.
alexmoreno’s picture

no problem, i will make the amends.

Thank you.

alexmoreno’s picture

Status: Needs work » Needs review
FileSize
4.02 KB

ready. Select box added instead textform and cleaned the code searching for possible problems with Drupal coding standards.

Regarding that last point, i found a comment in the wrong place, but I am not sure if the issue in the function which you are pointing is the set_error function:

    form_set_error('', t('You must select a numeric value.'));

I have modified it like that:

form_set_error('cpn', t('You must select a numeric value.'));

Let me know if it's right now please.

Thank you.

alexmoreno’s picture

sorry, found the issue with the Drupal coding standars in the tab. This is the right file.

Status: Needs review » Needs work

The last submitted patch, cpn-weightsettings-1912834-2.patch, failed testing.

alexmoreno’s picture

how/where can i check why is this failing?

alexmoreno’s picture

I don't understand the issue, I made the patch under bluefish/Ubuntu.

I will create the park again just using vim

DamienMcKenna’s picture

Status: Needs work » Needs review
FileSize
4 KB

The codebase had changed since you last pulled down the dev version.

I've made a few corrections to the setting, added a variable_del() line to hook_uninstall, and changed the weight values back to using JS_THEME rather than JS_LIBRARY. That said, I haven't tested it yet.

alexmoreno’s picture

that's why it was failing my simpletest?

I will test it for you, no problem. Regarding using JS_THEME or JS_LIBRARY, I am thinking in some situations in which the administrator could want to have their js being shown before the LIBRARY (like jquery?). Could that happen? Maybe it would be a good idea to give the possibility of choose this value (JS_THEME or JS_LIBRARY)?

Thanks for your help Damien.

DamienMcKenna’s picture

There's a separate issue for adding an option to control the scope: #1294896: Option to control the scope the JS/CSS are loaded in

alexmoreno’s picture

I see it. I'd love to help, if you like my approach I can work with that idea. Assign it to me if you think I can help.

Also, i wasthinking that it would be better idea to move this global weight to the node creation itself, so instead of having the same global variable for all the nodes,the author could decide which weight give to each node individually.

DamienMcKenna’s picture

@urwen: That'd be overkill, IMHO, you shouldn't need to monkey with the weight that much. If anything it might be worth expanding to have separate weights for the JS and CSS.

DamienMcKenna’s picture

FileSize
10.15 KB

@urwen: How about this... First off it lets you change the CSS and JS weights separately. Secondly, it lets you manually enter an exact number for the fields, rather than only being able to adjust it slightly. This should cover for all (normal) use cases.

alexmoreno’s picture

Sorry, I've been in holidays, in my home town in Spain... it's brilliant Damien, you have done a lot of good job.

I will test the path if that helps :-)

tusik’s picture

Is this patch already included in the Sept 30th 2013 dev version?

DamienMcKenna’s picture

@anficr: no.

tusik’s picture

Thank you.

alexmoreno’s picture

what do we need to include it in the next one?

DamienMcKenna’s picture

Issue summary: View changes
DamienMcKenna’s picture

Assigned: alexmoreno » Unassigned
Status: Needs review » Fixed

Committed! Thanks for the help!

DamienMcKenna’s picture

Status: Fixed » Needs review
FileSize
1.15 KB

I spotted a typo after committing the change.

DamienMcKenna’s picture

Status: Needs review » Fixed

Typo fixed :)

alexmoreno’s picture

Thanks Damien, glad to help :-)

DamienMcKenna’s picture

Version: 7.x-1.x-dev » 6.x-1.x-dev
Status: Fixed » Patch (to be ported)

Needs to be backported to D6.

DamienMcKenna’s picture

Issue tags: +needs backport to 6.x
alexmoreno’s picture

unfortunately, it cannot be done as Drupal 6 add_js and drupal_add_css does not support using a weight.

DamienMcKenna’s picture

DamienMcKenna’s picture

Version: 6.x-1.x-dev » 7.x-1.x-dev
Status: Patch (to be ported) » Closed (fixed)
Issue tags: -needs backport to 6.x

The D6 version of this module is no longer supported, so I'm closing this issue.