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

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.

BTW, I've updated the documentation a little - please find the attached
version.

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