Ticket/10811 AJAX callback "alt_text" insufficiently changes links #759
Conversation
| el.attr('title', alt_text); | ||
| el.text(alt_text); | ||
| }); | ||
|
|
||
| /** | ||
| * This callback is based on the callback above. |
There was a problem hiding this comment.
Specify which callback please, I'm sure at some point someone will add another function above this one.
|
What should probably be done here is toggle_subscribe callback should call alt_text callback and then perform additional actions needed. Implementing this will necessitate turning anonymous callback functions into named ones. |
|
I changed it, now the new callback calls the old one. |
| @@ -436,14 +436,40 @@ phpbb.add_ajax_callback = function(id, callback) | |||
| * the alt-text data attribute, and replaces the text in the attribute with the | |||
| * current text so that the process can be repeated. | |||
| */ | |||
| phpbb.add_ajax_callback('alt_text', function(data) { | |||
| phpbb.add_ajax_callback('alt_text', function() { | |||
| var el = $(this), | |||
| alt_text; | |||
|
|
|||
| alt_text = el.attr('data-alt-text'); | |||
There was a problem hiding this comment.
For data attributes you should use .data('alt-text') jQuery method I believe...
There was a problem hiding this comment.
It was changed from data() to attr() while the jquery PullRequest was made.
There was a problem hiding this comment.
Yeah, callumacrae already explained it to me. Ignore this note, please.
|
Looks good, except for toogle |
|
|
||
| phpbb.ajax_callbacks['alt_text'].call(this); | ||
|
|
||
| if (el.attr('href').indexOf('unwatch') > 0) |
There was a problem hiding this comment.
While this will probably always work, it should really either be >= 0 or > -1 or !== -1
|
Looks good. |
|
The usage of "alt" here is not ideal as "alt text" can be taken to mean "value of alt attribute". |
| */ | ||
| phpbb.add_ajax_callback('toggle_subscribe', function() { | ||
| var el = $(this), | ||
| alt_text; |
There was a problem hiding this comment.
As I think I said in the original ajax pr, each line should have its own var. I do see that you copy pasted this from elsewhere.
There was a problem hiding this comment.
Massive -1 to that. It is harder to read and uses considerably more resources.
There was an interesting talk in an IRC channel ages ago (I'll see if
I can find it later), where it was described as being almost like the
following:
var stuff = ['one'];
stuff.push('two');
stuff.push('three');Instead of var stuff = ['one', 'two', 'three'];.
Yes, mods can edit the last line, but it looks horrific and is inefficient.
On 1 May 2012, at 03:50, Oleg Pudeyev
reply@reply.github.com
wrote:
el.attr('title', alt_text);
el.text(alt_text);
});+/**
- * This callback is based on the alt_text callback.
- * It replaces the current text with the text in the alt-text data attribute,
- * and replaces the text in the attribute with the current text so that the
- * process can be repeated.
- * Additionally it replaces the icon of the link and changes the link itself.
- */
+phpbb.add_ajax_callback('toggle_subscribe', function() {- var el = $(this),
alt_text;As I think I said in the original ajax pr, each line should have its own var. I do see that you copy pasted this from elsewhere.
Reply to this email directly or view it on GitHub:
https://github.com/phpbb/phpbb3/pull/759/files#r754507
There was a problem hiding this comment.
I copied it form the callback above
There was a problem hiding this comment.
Well, it was wrong there and it is wrong here.
|
I can't see a problem with "alt_text". Most native English speakers On 1 May 2012, at 03:49, Oleg Pudeyev
|
|
|
||
| phpbb.ajax_callbacks['alt_text'].call(this); | ||
|
|
||
| if (el.attr('href').indexOf('unwatch') !== -1) |
There was a problem hiding this comment.
This is rather fragile. I would possibly prefer tracking the state in a data attribute instead.
There was a problem hiding this comment.
as we need to change the value in the link aswell, it doesn't really matter, whether its fragile?
There was a problem hiding this comment.
You would then switch between two server-side generated urls rather than assuming you know their contents on the client side.
There was a problem hiding this comment.
I'm going to rephrase what I said. There should be no url manipulation on the client side. Generate all urls on the server side and switch between them on the client side. To the client side urls should be opaque.
There was a problem hiding this comment.
Fixed everything
…l links PHPBB3-10811
* nickvergessen/ticket/10811: [ticket/10811] Make toogle_subscribe more generic so it can toogle all links [ticket/10811] Make it easier for MODs/Extensions to define the alt-text [ticket/10811] Make subscribe/unsubscribe repeatable with AJAX [ticket/10811] Fix AJAX callback alt_text so it can be repeated.
http://tracker.phpbb.com/browse/PHPBB3-10811