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

Cool - that works. Patch applied.

Thanks!

On Tue, Feb 27, 2018 at 6:30 AM, Harshal Dhumal <
harshal(dot)dhumal(at)enterprisedb(dot)com> wrote:

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

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

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