Re: RM2898 Keyboard navigation in dialog tabs (nav tabs)

Hi,

Please find the updated patch.

On Mon, Feb 26, 2018 at 8:03 PM, Dave Page <dpage(at)pgadmin(dot)org> wrote:

> Hi
>
> On Mon, Feb 26, 2018 at 12:03 PM, Harshal Dhumal <
> harshal(dot)dhumal(at)enterprisedb(dot)com> wrote:
>
>> Hi,
>>
>> Please find the updated patch for keyboard navigation.
>>
>> In this patch I have reduced delay which is required until current tab
>> navigation is completed.
>> Extracted class dialogTabNavigator and put it in new file.
>> Added jasmine test cases.
>> Fixed linting issues, variable naming convention issues.
>>
>
> This is still getting stuck on the Connection tab when I test on the
> server dialog.
>
The disabled input field (Host name/address) on connection tab was causing
issue.

>
> BTW, I've updated the documentation a little - please find the attached
> version.
>
Thanks for this. I have included revised version of documentation in
current patch.

>
> Thanks.
>
>
>
>>
>>
>>
>> --
>> *Harshal Dhumal*
>> *Sr. Software Engineer*
>>
>> EnterpriseDB India: http://www.enterprisedb.com
>> The Enterprise PostgreSQL Company
>>
>> On Wed, Feb 21, 2018 at 11:39 PM, Harshal Dhumal <
>> harshaldhumal15(at)gmail(dot)com> wrote:
>>
>>> Hi,
>>>
>>> On Wed, Feb 21, 2018 at 10:59 PM, Joao De Almeida Pereira <
>>> jdealmeidapereira(at)pivotal(dot)io> wrote:
>>>
>>>> Yep I installed the V2 file
>>>>
>>>> On Wed, Feb 21, 2018 at 11:31 AM Dave Page <dpage(at)pgadmin(dot)org> wrote:
>>>>
>>>>> On Wed, Feb 21, 2018 at 4:22 PM, Joao De Almeida Pereira <
>>>>> jdealmeidapereira(at)pivotal(dot)io> wrote:
>>>>>
>>>>>> Hello Harshal,
>>>>>>
>>>>>> I passed the patch through our CI and all the tests passed. The
>>>>>> changes do not break previous behavior but because there are no tests on
>>>>>> the new feature we could not be sure it was really working. So we did some
>>>>>> manual testing and sometimes it doesn't work, like it gets stuck in a place
>>>>>> and you need to press the shortcut again in order for it to move.
>>>>>>
>>>>>
>>>>> It stuck because I have to wait until next tab is completely visible
>>> (fade in effect is completed).
>>> The fade in or fade out transition duration is 150 ms (set by
>>> bootstrap). So I can not set focus back to tab pane
>>> until fade in or fade out transition is completed. May be one
>>> improvement I can do is to reduce wait time to
>>> something 200 ms (currently it's 500 ms).
>>>
>>> Note that the original issue reported by Dave is already fixed in
>>> updated patch.
>>>
>>>
>>>> Was that with the updated patch? It sounds like the issue I saw with
>>>>> the original one.
>>>>>
>>>>>
>>>>>>
>>>>>> Codewise I have some suggestions:
>>>>>> - dialogTabNavigator looks a nice candidate for a class with its own
>>>>>> file. This way we can test the behavior
>>>>>>
>>>>> Ok I'll move dialogTabNavigator to new file and will add test cases.
>>>
>>>> - There is no difference between a variable called e and a variable
>>>>>> error so for sake of clarity I would love to see variable names that
>>>>>> we can easily read
>>>>>> - We are also using 2 different types of variable naming camelCase
>>>>>> and snake_case, if we could use only camelCase on Javascript it would make
>>>>>> the code more uniform
>>>>>>
>>>>> Ok I'll do this.
>>>
>>>
>>>> - I noticed that there are some linting issues in the Javascript code
>>>>>>
>>>>> I just found that linter was disabled for this file by adding comment
>>> /* eslint-disable */ at first line. (not sure why we did that)
>>>
>>>
>>>>>> Summing it up I believe that despite the feature not working 100% of
>>>>>> the time, looks like the code is almost there but needs some refactoring to
>>>>>> make it more readable, instead of comments we could have function calls
>>>>>> that more clearly state what we are looking for something like
>>>>>> DialogTabNavigator.isLastTabOfChild
>>>>>> ​
>>>>>> ​
>>>>>>
>>>>>>
>>>>>> Thanks
>>>>>> Joao
>>>>>>
>>>>>> On Wed, Feb 21, 2018 at 4:32 AM Harshal Dhumal <
>>>>>> harshal(dot)dhumal(at)enterprisedb(dot)com> wrote:
>>>>>>
>>>>>>> Hi,
>>>>>>>
>>>>>>> --
>>>>>>> *Harshal Dhumal*
>>>>>>> *Sr. Software Engineer*
>>>>>>>
>>>>>>> EnterpriseDB India: http://www.enterprisedb.com
>>>>>>> The Enterprise PostgreSQL Company
>>>>>>>
>>>>>>> On Tue, Feb 20, 2018 at 10:34 PM, Dave Page <dpage(at)pgadmin(dot)org>
>>>>>>> wrote:
>>>>>>>
>>>>>>>> Hi
>>>>>>>>
>>>>>>>> On Tue, Feb 20, 2018 at 7:22 AM, Harshal Dhumal <
>>>>>>>> harshal(dot)dhumal(at)enterprisedb(dot)com> wrote:
>>>>>>>>
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> Please find attached patch to enable keyboard navigation in dialog.
>>>>>>>>>
>>>>>>>>> To allow navigation from one tab pane (bootstrap tab pane) to
>>>>>>>>> another one
>>>>>>>>> I have added two new shortcut preferences *Dialog tab previous *(
>>>>>>>>> shift+control+[ ) and *Dialog tab next* ( shift+control+] ) for backward
>>>>>>>>> and forward tab navigation.
>>>>>>>>>
>>>>>>>>> Also all dialog controls (within same tab pane) can be navigated
>>>>>>>>> using TAB key.
>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>>> This seems unreliable to me - for example, it keeps getting stuck
>>>>>>>> on the connection tab on the server properties dialog.
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>>> Also, can we use the same wording as for the tabbed panel
>>>>>>>> navigation please? E.g. Next/Previous instead of Forward/Back.
>>>>>>>>
>>>>>>>
>>>>>>> I have fixed all of above issues. Please find updated patch.
>>>>>>>
>>>>>>> Thanks,
>>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>> --
>>>>>>>> Dave Page
>>>>>>>> Blog: http://pgsnake.blogspot.com
>>>>>>>> Twitter: @pgsnake
>>>>>>>>
>>>>>>>> EnterpriseDB UK: http://www.enterprisedb.com
>>>>>>>> The Enterprise PostgreSQL Company
>>>>>>>>
>>>>>>>
>>>>>
>>>>>
>>>>> --
>>>>> Dave Page
>>>>> Blog: http://pgsnake.blogspot.com
>>>>> Twitter: @pgsnake
>>>>>
>>>>> EnterpriseDB UK: http://www.enterprisedb.com
>>>>> The Enterprise PostgreSQL Company
>>>>>
>>>>
>>>
>>
>
>
> --
> Dave Page
> Blog: http://pgsnake.blogspot.com
> Twitter: @pgsnake
>
> EnterpriseDB UK: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>