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. BTW, I've updated the documentation a little - please find the attached version. 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
keyboard_shortcuts.rst
Description: Binary data
