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
