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

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