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

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.

Codewise I have some suggestions:
- dialogTabNavigator looks a nice candidate for a class with its own file.
This way we can test the behavior
- 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
- I noticed that there are some linting issues in the Javascript code

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