Hi, I'm still able to make it get stuck, if I tab back and forth quickly.
On Mon, Feb 26, 2018 at 6:04 PM, Harshal Dhumal < harshal.dhu...@enterprisedb.com> wrote: > Hi, > > Please find the updated patch. > > On Mon, Feb 26, 2018 at 8:03 PM, Dave Page <dp...@pgadmin.org> wrote: > >> Hi >> >> On Mon, Feb 26, 2018 at 12:03 PM, Harshal Dhumal < >> harshal.dhu...@enterprisedb.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 < >>> harshaldhuma...@gmail.com> wrote: >>> >>>> 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 >>>>>> >>>>> >>>> >>> >> >> >> -- >> 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
