This task falls mainly under "code maintenance".

This patch introduces no new features. It handles the following:

1. Removing drupal_add_css|js() from the .tpl

Our CSS/JS was loaded in the .tpl file. That's a legacy from the D5 days: we wanted to make life easy for themeres (D5 didn't have .info where one could declare his own CSS). But this is not a conventional Drupal technique, it may be faulty, and this may tie our hands in the future because we lose control over our own files (for example, say we want to split or rename them).

2. Symplifying the logic

The logic for determining whether to load the files was a bit arcane, and wouldn't apply for 3rd party link-types. This is now replaced by simple 'uses standards css' and 'uses standards js' flags on the link-type definitions.

3. Fixing a subtle bug

We used a $first_time variable for optimizing the CSS/JS inclusion. But each link-type needs its own $first_time variable. So this is now replaced by an $initialized array.

CommentFileSizeAuthor
css.diff4.09 KBmooffie

Comments

amitaibu’s picture

amitaibu
Primary language Hebrew
Location Israel
commented 
+++ flag.module
@@ -320,10 +320,14 @@ function flag_flag_link_types() {
+      'uses standard js' => TRUE,
+      'uses standard css' => TRUE,

I think the name "standard js/css" isn't very clear. Also, I think that it's more understandable/ predictable if the values would have been a declaration to the JS/ CSS file:

'js' => drupal_get_path()...

If we'll want one day an alter function, it will be easier for implementing modules to alter the "standard" JS/ CSS files.

Powered by Dreditor.

quicksketch’s picture

quicksketch
he/him
commented

I'd agree with Amitaibu here, we should probably just use an array of attached JS/CSS files, similar to a D7 #attached array. It's unlikely that we'll need to move or change the default files within the 2.x branch of Flag. If we absolutely need to split or move these files we could wait until a 3.x release of Flag to break things.

That said, I think removing the CSS/JS from the template file is a good thing. It certainly is not a best practice within Drupal 7, which should always use an #attached array on the renderable thing being output.

mooffie’s picture

mooffie commented
Status: Needs review » Closed (duplicate)

it's more understandable/ predictable if the values would have been a declaration to the JS/ CSS file:
'js' => drupal_get_path()...

we should probably just use an array of attached JS/CSS files, similar to a D7 #attached array.

I agree. I suggested it in the other issue.

But that was not the purpose of _this_ patch.

There's some misunderstanding (my fault):

The purpose of this patch was, also, to clear the way for further work (if desired). Writing this patch was the shortest way for me to explain "($flag->link_type == 'toggle' || $user->uid == 0) && $first_time" and the subtle bug. The purpose wasn't to implement the CSS inclusion described in Amitai's thread. I was on purpose avoiding that here (because it potentially requires a lengthy discussion).

I'm marking this a dup of #858764: Improve API to allow implementing modules easier access to flag

(Which I think is a pity: Working in stages is a good thing, IMO. Amitai, move this back to 'needs review' if you think it's going to make your work easier; but I'm not planning to tackle the #attach (or whatever) thingy here.)

amitaibu’s picture

amitaibu
Primary language Hebrew
Location Israel
commented
Status: Closed (duplicate) » Needs review

re-opening in light of discussion in #858764: Improve API to allow implementing modules easier access to flag

mooffie’s picture

mooffie commented

Nate, "the ball is in your field" now.

Do you approve of this patch as-is? Amitai does.

Let me explain:

Implementing #attached is a nice thing, but it involves some deliberations, and it will probably ease matters if we decouple these preliminary cleanups from it.

(In Drupal's Core features are usually committed in one big patch, not in several small steps (each having merit in and of itself), but this is because of some very good reasons of which none necessarily apply to contribs.)

If you disagree, that's fine; I'll mark this "duplicate" again.

mooffie’s picture

mooffie commented
Title: Streamline the CSS / JS inclusion » Cleanup the CSS / JS inclusion

I'm not an English speaker: probably the "streamline" word I used was incorrect.

mooffie’s picture

mooffie commented

we should probably just use an array of attached JS/CSS files, similar to a D7 #attached array.

The purpose here isn't to introduce new features nor to backport D7 ones. The purpose is:

- To clean up our spaghetti code.

- To fix minor bugs in it.

- By this, to clear the way for others programmers (and ourselves) to approach our code.

Currently, able programmers who try to approach our code fail: they put their code in the wrong if() {} branch. No surprise.

I'd agree with Amitaibu here, we should probably just use an array of attached JS/CSS files, similar to a D7 #attached array.

We all agree about this Noble Plan. But how is it practical to have a patch that deals with both the #attached feature and these fixes? Think about the philosophical discussions that accompany a patch: Then multiple it by the number of sub-issues we have here. Then you'll have the Flag 2.0 release date: Never.

To sum it up:

Nate, are you in favor of this patch as-is ?

(If you aren't, I'll "wontfix" this, which will be sad. I'm in favor of committing this and going forward.)

mooffie’s picture

mooffie commented

Nate: I'll commit this in a couple of days, unless you veto this.

mooffie’s picture

mooffie commented
Status: Needs review » Fixed

Committed.
http://drupal.org/cvs?commit=491588
http://drupal.org/cvs?commit=491590

I like this patch. It makes it easy to see what's going on, and to change it. (E.g., we can now easily include the CSS for normal links too, if we want to.)

Status: Fixed » Closed (fixed)

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