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

Hi,

On Tue, Feb 27, 2018 at 1:08 AM, Dave Page <dpage(at)pgadmin(dot)org> wrote:

> Hi,
>
> I'm still able to make it get stuck, if I tab back and forth quickly.
>
Quickly switching tabs was causing to switch to next tab before previous
navigation was completed and
this was leading to lose focus on tab pane.
Now I have made changes that it won't process any user navigation events
until current navigation is completed.

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