[Phpmyadmin-devel] Ajaxification of PMA

[Rouslan Placella]

> On Sun, Mar 18, 2012 at 5:06 PM, Rouslan Placella <rouslan at placella.com>wrote:
> 
>> On 18/03/12 11:08, Thilanka Kaushalya wrote:
>>> Hi Rouslan,
>>>
>>> Hi Thilanka,
>>>>
>>>> off the top of my head I can think of an issue that can be solved with
>>>> some ajaxification. On tbl_structure page, if you use the "Add * index"
>>>> actions that are available in the "More" dropdown corresponding to a
>>>> column in the top table. You will be dropped to sql.php, which will show
>>>> the table structure page, but it will not include "tbl_structure.js", so
>>>> all the ajax actions on this page will be broken. Of course, if the "Add
>>>> * index" links would be ajaxified, this would no longer be a problem,
>>>> because we would not leave the tbl_structure page.
>>>>
>>>> Please do not take the following badly, but rather as constructive
>>>> criticism and as a way to learn from your mistakes. I find that some of
>>>> the ajaxification work that you did this summer was less then optimal.
>>>> As an example, let's look at some issues with the work that you did on
>>>> the index editor.
>>>>
>>>> First of all, you create a custom slider to hide the indexes table. This
>>>> was unnecessary, because we already have a PHP function that takes care
>>>> of this, which also respects the $cfg['InitialSlidersState']
>>>> configuration setting.
>>>>
>>>> Another issue was that in every ajax reply the top menu was included and
>>>> then removed from the reply via jQuery. This, of course, causes transfer
>>>> of waste data and it's better not to send it in PHP altogether.
>>>>
>>>> Then there was an issue when a user requested to add a column to the
>>>> index editor: the browser fired an HTTP request of the form to the
>>>> server, just to get the same form back with an extra empty field. Of
>>>> course, this action can be done on the client side by just cloning a row
>>>> from the table.
>>>>
>>>> So, I fixed all of the above and I also cleaned up the layout of the
>>>> editor in order to use less horizontal space and appear more
>>>> user-firendly. I invite you to look at these changes so that you can
>>>> learn from them. It's commit 863819cbdd759 and the following 10 commits
>>>> after that.
>>>>
>>>> By the way, if you need a mentor or just some guidance, please feel free
>>>> to contact me at any time. I would be glad to help you with any issues
>>>> you may have and/or review your code.
>>>>
>>>> Bye,
>>>> Rouslan
>>>>
>>>>
>>> Sorry for the delayed reply I'm sending. I went through your above
>>> mentioned commits and observed how was it implemented. Now I realize that
>>> some of the previous work I did have optimization problems. I'll go
>> through
>>> the list of functionality I have implemented and come up with a optimized
>>> solution for them. Thanks for the guidance you gave.
>>
>> Glad I could be of help ;)
>>
>> Rouslan

> On 18/03/12 15:14, Thilanka Kaushalya wrote:
>> Hi Rouslan,

Hi Thilanka,

We have internally agreed that we will use bottom-posting [0] on this
mailing list, please follow this.

>> My first task of the gsoc session was ajaxifying the "Create Table" option.
>> So I went through the functionality and refactored the code. I have
>> submitted the patch with the id 3507868 [0].

I personally wouldn't call that refactoring, but yeah, the clean-up
looks pretty OK. Just one thing: when you are calling
PMA_ajaxShowMessage() to display an error, you need to pass a second
parameter with a false value. For example:

PMA_ajaxShowMessage(data.error, false);

More info at [1].

>> With this option I see some issues.
>> 
>>    1. Cancel button is configured with the
>>    "$(this).dialog('close').remove();" option and I think "remove()"
>>    is necessary because if not it will duplicate the dialog options. You can
>>    check it by using the create table option more than once without reloading
>>    the page. At that case second time the "Cancel" button will not display if
>>    you keep only the "$(this).dialog('close')" part.

I never said that you should not remove the dialog, you're just doing it
wrong. The correct way to do this is by defining a callback that gets
fired when the dialog is closed (this may happen in more than one way)
and remove the dialog there. See example below.

>>    2. The style of the Save button and Cancel button seems different. It is
>>    effecting to the consistency of the UI.

That because the buttons are not where they belong. They should go into
the bottom pane of the dialog. For example:

// Define some buttons that will go into the bottom pane
// of the dialog and bind some callbacks to them
var buttonOptions = {};
buttonOptions[PMA_messages['strGo']] = function () {
    // do something useful here
};
buttonOptions[PMA_messages['strClose']] = function () {
    $(this).dialog("close");
};
// Create the dialog
$someDiv.dialog({
    buttons: buttonOptions,
    close: function () {
        // This is a callback that gets fired when the user
        // or the system try to close the dialog
        $(this).remove();
    }
});

>> Can yo check with this patch can comment on the changes. Thank you.
>> 
>> Regards,
>> Thilanka.
>> 
>> [0] -
>> https://sourceforge.net/tracker/?func=detail&aid=3507868&group_id=23067&atid=377410

Bye,
Rouslan

[0]: http://en.wikipedia.org/wiki/Posting_style#Bottom-posting
[1]:
https://github.com/phpmyadmin/phpmyadmin/blob/QA_3_5/js/functions.js#L1250