We currently have:

    function flagClick(element, settings) {
      ...
      $.ajax({
        ...
        success: function (data) {
          // Display errors
          if (!data.status) {
            // Change link back
            flipLink(element, settings);
        ...
      });
      // Swap out the links.
      flipLink(element, settings);
    }

But the 'element' that flipLink() uses, under the "Change link back" line, no longer exists: the outer flipLink(), under the "Swap out the links" line, swapped it out of the DOM.

(This bug seems negligible, but the bigger issue here is that we lose the element --and I need to have it, for a future feature.)

CommentFileSizeAuthor
#3 flag_saveelement.diff3.87 KBmooffie

Comments

mooffie’s picture

mooffie commented
Assigned: Unassigned » mooffie

Assigning to self.

quicksketch’s picture

quicksketch
he/him
commented

I just ran into this bug also when working on making the flag() function work with comments. If Flag returns back { status: false }, that second flipLink() causes a JS error. So this would be great to fix to help gracefully fail.

mooffie’s picture

mooffie commented
StatusFileSize
new flag_saveelement.diff3.87 KB

Here's a demonstration patch, but it's only the starting point (so I'm not marking this 'needs review'). Support should yet be added to 'links' that don't contain a link at all (for example, if we theme the link to show a static text: "You bookmarked this post, thanks!").

mooffie’s picture

mooffie commented

BTW, this issue is much more important than it seems.

The purpose here is to make the 'success' handler have access to the element. This gives us, in the near future, the ability, in this handler, to pull the link's texts from the server instead of, or in addition to, storing them in a huge JS structure. This is needed if our links are brought into the page via ajax, so this is need for Views2's ajax feature. This is also needed for two features I'm planning.

mooffie’s picture

mooffie commented

Support should yet be added to 'links' that don't contain a link at all

I think we should leave that to a different issue, because else the code will get more complicated and the review process might be lengthy.

So I'm marking this patch 'needs review'.

You may be tempted to think that the old code _does_ handle 'links' that don't contain an <a> tag, because it has:

if ($('a', $newLink).size() > 0) {
  ...
}
else {
  ...
}

but this isn't so: this structure just means "if there's no <a> whithin, then it's surely the outer element that's the <a>".

mooffie’s picture

mooffie commented
Status: Active » Needs review
mooffie’s picture

mooffie commented

$newLink.find('a.flag, a.unflag')

Note that I don't do $newLink.find('a') because there might be links there that we shouldn't touch, e.g. the themer may put a help link there.

quicksketch’s picture

quicksketch
he/him
commented

Another option is to simply not keep all that data on the page at load-time. In our AJAX request, return the needed information back to the page. It'll be a bit slower, but personally I don't find the immediate toggling that helpful, and it makes me feel like nothing happened.

mooffie’s picture

mooffie commented
Status: Needs review » Closed (duplicate)

This issue is superseded by:

#313862: Fix, and enhance, the AJAX support