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