There are several issues with our markup.

First, let's refresh our memory. Our markup is:


<span class="flag-wrapper">

  <a href="..." class="{flag|unflag} flag-/flagname/ [unflagged|flagged]">
    Flag this!
  </a>

  <span class="flag-message flag-{flag|unflag}-message">
    Thanks!
  </span>

</span>

The issues:

  1. Nate suggested to get rid of the huge JS structure. This will simplify our code, which is a good thing. However, we will then lose the instant feedback. Perhaps we should have some "wait" indicator as compensation? (Perhaps borrow Drupal 6's "throbber.gif", the running dial?). I don't feel strongly about this, so it's fine with me if we don't have an indicator.
  2. Our '.flag-message' class uses 'position: absolute' to position the flag message in the "right place". The '.flag-wrapper' is established as the 'containing block' of this message --by adding 'position: relative' to it. Problem is, this establishment fails in Opera. My experience teaches me that "Opera is always right", so it may mean we have some error in our CSS. I should refresh my memory of the CSS specification, but perhaps some reader here already knows something about this. (I suspect it's because {span} is an inline element.) I mention this issue for one reason only: so that the "wait" indicator, mentioned above, is done right from the start.
  3. Nate mentioned IE6 not supporting '.class1.class2' in CSS selectors. So he suggests moving the 'flag-/flagname/' class to the wrapper.
  4. We use 'flag' and 'unflag' classes. This has two problems:
    1. 'flag' is a verb here. It might confuse people because it looks like a noun, so we're going to waste energy explaining to people that it's a verb that stands for the action. We can instead rename these two to 'action-flag' and 'action-unflag', respectively.
    2. Eventually we should use 'Drupal.behaviors' so that our click handlers are attached in Ajax scenarios as well. Our selector could then be:
        // This was not tested, but let's assume it's correct:
  $('.flag-wrapper').find('a.flag, a.unflag').not('.flag-processed')

  // Alternatively, we could use the following. But it wouldn't work
  // if the wrapper contained some non-flagging link, e.g. a help link.
  $('.flag-wrapper a:not(.flag-processed)')

      This selector(s) is "complicated" because we don't have a fixed class on our links (we have two possibilities: either 'flag' or 'unflag'). This isn't a problem. But if we do rename the existing 'flag' to 'action-flag', we could then use 'flag' as the fixed flag.

CommentFileSizeAuthor
#3 flag_newjs.diff11.11 KBmooffie

Comments

mooffie’s picture

mooffie commented

$('.flag-wrapper').find('a.flag, a.unflag').not('.flag-processed')

(Correction: for efficiency, this should be 'span.flag-wrapper', not '.flag-wrapper'.)

The reason I start with this 'span.flag-wrapper' is for efficiency: that not all {A} elements in the document are traversed twice. Efficiency is another reason why having a fixed class on our {A}s is a good thing; and we could simply do $('a.flag:not(.flag-processed)').

mooffie’s picture

mooffie commented
Assigned: Unassigned » mooffie

I'll be working on this.

mooffie’s picture

mooffie commented
Status: Active » Needs work
StatusFileSize
new flag_newjs.diff11.11 KB

Here the patch so far.

  1. Here's a map of the new CSS classes:
    <span class="flag-wrapper flag-/flagname/">

  <a href="..." class="flag action-{flag|unflag} [unflagged|flagged]">
    Flag this!
  </a>

  <span class="flag-message flag-{flag|unflag}-message">
    Thanks!
  </span>

</span>

    In other words, there are three changes here (all discussed above):
    (1) I moved 'flag-/flagname/' to the wrapper,
    (2) There's now a fixed class on the link, 'flag'.
    (3) The old 'flag' and 'unflag' were renamed to 'action-flag' and 'action-unflag'.

  2. The huge JS structure is gone. Links are updated through ajax reponse. Benefits:
    (1) Links should work in 'Use AJAX' views (but right now they don't: it seems there's a bug in Views; I need to investigate).
    (2) The [flag-bookmark-count] token is "dynamic". Cool.
  3. Things To Do:
    (1) I was thinking about adding a throbber, but since the waiting time is short, that throbber may actually have negative aesthetic effect --when displayed inline. So pehaps I'll show it in 'posiition: absolute', but then I first need to:
    (2) Investigate the 'postition: absolute' problem in Opera.
quicksketch’s picture

quicksketch
he/him
commented

Looks great! I honestly prefer the message being fed from AJAX instead of being instant anyway. I do think adding feedback will be necessary though. I think the /misc/throbber.gif is suitable for this, though I'm not sure if absolute or relative positioning would be better. Lastly, if we can name the class "flag-action" instead of "action-flag" that would be better IMO. We're bending the rules a little bit with prefixing classes with both "flag" and "unflag", but generally prefixing classes with the module name is convention.

mooffie’s picture

mooffie commented

(I wrote that our CSS fails in Opera and I suggested our CSS is faulty, but it turns out the problem is actually in Opera.)

mooffie’s picture

mooffie commented

I'm glad you like the patch so far. It makes our JS much simpler.

I think the /misc/throbber.gif is suitable for this, though I'm not sure if absolute or relative positioning would be better

Views 2 too uses this throbber. It uses the simplest technique: it shows it inline. But, naturally, this pushes forward the content that happens to follow it. And if the ajax executes fast, this content goes back promply. I was wondering whether this "jump" is unaesthetic. I haven't actually tried this; I'm only imagining it in my mind's eyes. Can you imagine it your mind too? How does it look like to you? :-) LOL.

quicksketch’s picture

quicksketch
he/him
commented

I don't mind the jump terribly much, especially since the width will jump when the new "unflag" link comes in anyway. Perhaps we could use the following mechanism:

- User clicks link
- Link is removed from display, replaced with "[throbber] Saving..."
- Link is restored with new text

If possible (big if), we can set the width of the field to a static px width to prevent the first jump at all, then animate the width difference between the "Saving..." and the display of the new link. I'm not sure how well this will work because you can't set the width on an element displayed inline, perhaps the inline-block display would work for us (in IE6/IE7/FF3/Safari/Opera, but not FF2, where we'd have to keep the jumpiness).

mooffie’s picture

mooffie commented

I don't mind the jump terribly much, especially since the width will jump when the new "unflag" link comes in anyway.

Aha, I neglected to take that into acount. Your imagining powers are robust. So perhaps the "jump" is negligible, after all. We'll soon see.

If possible (big if), we can set the width of the field to a static px width to prevent the first jump at all,

That's worth trying. We don't really need to set the width: a simple "visibility: hidden" on the link should work (and we'll make a reasonable assumption that "[throbber] Saving..." will be shorter than the flag/unflag text).

It's weekend, so I won't publish a revised patch very soon.

If you think of more ideas to express this "saving..." indicator, please post them here.

mooffie’s picture

mooffie commented

if we can name the class "flag-action" instead of "action-flag" that would be better IMO.

OK, I changed that.

(I actually think "action-{flag|unflag}" is better than "{flag|unflag}-action", but we'll discuss this later, perhaps, it isn't that important.)

<span class="flag-message flag-{flag|unflag}-message">
  Thanks!
</span>

Note: I'm changing this "flag-{flag|unflag}-message" to "flag-{unflagged|flagged}-message" because the current class is very unintuitive (I'll explain in more length if you want me to). A bonus: This way it will match the {unflagged|flagged} class on the link itself.

mooffie’s picture

mooffie commented
Status: Needs work » Postponed

An offshoot issue:
#282006: Have a 'flag.tpl.php'

I'm marking this 'postponed' till that other issue gets settled. (The purpose is to make it easier for me to touch our markup, and the current theme_flag_link() doesn't make it easy.)

mooffie’s picture

mooffie commented
Status: Postponed » Closed (fixed)

Most of the problems described here have been fixed already.

And the throbber is dealt in:

#313862: Fix, and enhance, the AJAX support

So I'm closing this issue.