Hi, On Wed, Feb 21, 2018 at 10:59 PM, Joao De Almeida Pereira < jdealmeidapere...@pivotal.io> wrote:
> Yep I installed the V2 file > > On Wed, Feb 21, 2018 at 11:31 AM Dave Page <dp...@pgadmin.org> wrote: > >> On Wed, Feb 21, 2018 at 4:22 PM, Joao De Almeida Pereira < >> jdealmeidapere...@pivotal.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.dhu...@enterprisedb.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 <dp...@pgadmin.org> wrote: >>>> >>>>> Hi >>>>> >>>>> On Tue, Feb 20, 2018 at 7:22 AM, Harshal Dhumal < >>>>> harshal.dhu...@enterprisedb.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 >> >
