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

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

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

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

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