Cleaned up the showEditDialog Function #1331
Conversation
This function had some odd code in it, using recursion to fetch data. So I updated it, should fix the problem where you had to click twice on the edit cog for an edit dialog to display.
| data: $(parent.editFormId).serializeArray(), | ||
| success: function (resp) { | ||
| $('#editdialog').dialog('close'); | ||
| $.when($.ajax(jsAjaxServer + '/ajax.server.php?page=tag&action=get_tag_map'), $.ajax(jsAjaxServer + '/ajax.server.php?page=tag&action=get_labels')).then(function( a1, a2 ) { |
There was a problem hiding this comment.
Should be splitted across multiples lines.
Also, a1 and a2 are not really clear names. Why not xhr1 and xhr2 or response1 and response2?
There was a problem hiding this comment.
Also, this means we are doing the two requests every time we open the dialog box, isn't it?
I mean the
if (tag_choices == undefined && tag_choices != '')
and
if (label_choices == undefined && label_choices != '')
have disappeared.
Cannot we replace $.ajax with our own Promise doing an ajax call if necessary and returning directly otherwise?
There was a problem hiding this comment.
The ifs were useless
And I'm unsure about that Promise part, care to show me an example.
There was a problem hiding this comment.
As far as I understand, tag_choices is empty at the first run, then, the if ensures that we load them from the AJAX call. On the subsequent runs, tag_choices is no longer empty and the AJAX call is not done anymore.
I have the impression that we are making an AJAX call systematically with the refactor, whereas we were saving one call with the already existing version. Isn't it?
|
Did a quick review. Noticed issues are mostly details. I tested it on my instance and it works great! |
|
When I get a second I'll fix the small problems. Can you show me how error handing should be done though |
|
Typically ampache/lib/javascript/dynamicpage.js Line 46 in c053d43 Thanks |
|
I haven't been very active in Github of lates. What was needed in this request to be complete? |
|
Me neither @AshotN 😄. Phyks explained the changes to be done on July 15. You should display "other commented on an outdated diff" by clicking on such links. |
|
How's that looking @Afterster and @Phyks? |
|
Unfortunately @Phyks comments are still valid, specifically about saving Ajax calls (tag_choices and label_choices checks). |
|
I'm not sure I'm understanding where we are making excessive Ajax calls. |
|
closing as this was added previously so there's no reason to keep it open. |
This function had some odd code in it, using recursion to fetch data. So I updated it, should fix the problem where you had to click twice on the edit cog for an edit dialog to display.