On Mon, Jun 16, 2014 at 10:20 PM, Isaac Bennetch <bennetch@gmail.com> wrote:


On 6/16/14 12:31 PM, Chirayu Chiripal wrote:
> On Mon, Jun 16, 2014 at 8:02 PM, Chirayu Chiripal
> <chirayu.chiripal@gmail.com <mailto:chirayu.chiripal@gmail.com>> wrote:
>
>
>     On Mon, Jun 16, 2014 at 7:53 PM, Isaac Bennetch <bennetch@gmail.com
>     <mailto:bennetch@gmail.com>> wrote:
>
>
>
>         On 6/16/14 9:59 AM, Chirayu Chiripal wrote:
>         >
>         >
>         >
>         > On Mon, Jun 16, 2014 at 7:14 PM, Isaac Bennetch
>         <bennetch@gmail.com <mailto:bennetch@gmail.com>
>         > <mailto:bennetch@gmail.com <mailto:bennetch@gmail.com>>> wrote:
>         >
>         >     Hello,
>         >
>         >     On 6/16/14 8:09 AM, Chirayu Chiripal wrote:
>         >     > Hello Isaac Sir,
>         >     >
>         >     > Please review this branch:
>         >     https://github.com/D-storm/phpmyadmin/tree/FR-755
>         >     >
>         >     > Tasks to be done yet (Will be pushed soon):
>         >     >
>         >     > 1) Fix failing test cases and improve test cases.
>         >
>         >     Yes, this is quite good but I have a few minor suggestions.
>         >
>         >     1) There should be some way to inform the user once
>         they've selected
>         >     this option. I have a few ideas about this but don't
>         prefer one over the
>         >     other at the moment.
>         >
>         >     What do you think of adding a checkbox in front of the
>         text "Change
>         >     Database/Table/Column names". When the user clicks the
>         checkbox or the
>         >     text, the dialog appears; once the dialog is closed the
>         checkbox is
>         >     checked.
>         >
>         >     An alternative is making an icon appear in front of the
>         text (a green
>         >     checkbox, probably) once the dialog is closed.
>         >
>         >     I think it's important to give visual feedback that the
>         feature is going
>         >     to be active.
>         >
>         >
>         > I think I'll go with green checkbox.
>         >
>         >
>         >     2) I suggest adding a "reset" button to the dialog which
>         would clear all
>         >     values. I also think the upper-right X (close button)
>         should abandon any
>         >     changes made; right now it saves those changes.
>         >
>         >
>         > Actually, there is nothing like save here, those input fields
>         (initially
>         > hidden) are just displayed when dialog opens and hides when dialog
>         > closes but I'll see how can it be done. Should it reset every
>         field
>         > i.e. all databases/tables/columns inputs?
>         > Also, then we should change "close" button to "save & close".
>
>         Ah, I see. What does everyone else think -- should we allow the
>         user to
>         reset to a blank dialog, and if so should it affect only the
>         displayed
>         table or all tables? I think a button with text "Reset all" that
>         resets
>         all of the form fields might be the best way here, but of course I'm
>         open to other suggestions.
>
>         "Save and close" was my original thought, but now that I see how the
>         form works I have doubts (changes are saved immediately rather
>         than when
>         pressing the "Save and close" button). To me, when I saw "Close" by
>         itself I wasn't sure if that meant it would accept my changes or
>         discard
>         them -- but I didn't realize changes are essentially saved
>         immediately.
>
>         Here's what I think we should ideally do, without thinking about
>         whether
>         it's actually possible or easy to accomplish:
>
>         * A "Reset all" button that resets all possible rename fields to
>         blank.
>         * Renaming the "Close" button to "Save and close"
>         * Removing the upper-right X of the dialog so that we remove the
>         question about what happens in this case (as a user, I would
>         think it
>         discards changes without saving; but doing so isn't possible
>         with this
>         dialog type so I'd rather remove the button entirely)
>         * Detecting when any value is present in any of the fields and
>         displaying the green checkbox in those cases; if the user
>         removes all
>         "renamed" fields so nothing is renamed, the checkbox should
>         disappear.
>
>         How does this sound?
>
>
>     Great. Cleared all my doubts. I think might add another "reset"
>     button which will clear visible fields only.
>
>
>         >     3) I think we should call this "Rename exported
>         database/column/table
>         >     names" -- but I'm open to other suggestions.
>         >
>         >
>         > This sounds good enough. I will change it soon.
>         >
>         >
>         >     4) I've tested with many columns and the dialog properly
>         adjusts
>         >     downward, so this is good behavior.
>         >
>         >     I'll make some comments in GitHub as well.
>         >
>         >     Thanks.
>         >
>
>
> I have made the suggested changes. Please have look a at it and let me
> know if I missed something.

Very nice work! I'm happy with it and think it's ready.

I'll soon be sending a PR after squashing commits.
 

>
>         >
>         >     > --
>         >     > Regards,
>         >     > Chirayu Chiripal
>         >     > phpMyAdmin Intern - Google Summer of Code 2014
>         >     > https://chirayuchiripal.wordpress.com/
>         >
>