Cool - that works. Patch applied. Thanks!
On Tue, Feb 27, 2018 at 6:30 AM, Harshal Dhumal < harshal.dhu...@enterprisedb.com> wrote: > Hi, > > On Tue, Feb 27, 2018 at 1:08 AM, Dave Page <dp...@pgadmin.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.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 >> > > -- Dave Page Blog: http://pgsnake.blogspot.com Twitter: @pgsnake EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company
