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

Hi,

I'm still able to make it get stuck, if I tab back and forth quickly.

On Mon, Feb 26, 2018 at 6:04 PM, Harshal Dhumal <
harshal(dot)dhumal(at)enterprisedb(dot)com> wrote:

> 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
>>
>
>

--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company