Thread: RM2898 Keyboard navigation in dialog tabs (nav tabs)
Attachment
Hi,Please find attached patch to enable keyboard navigation in dialog.To allow navigation from one tab pane (bootstrap tab pane) to another oneI 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.
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake
EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
HiOn Tue, Feb 20, 2018 at 7:22 AM, Harshal Dhumal <harshal.dhumal@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 oneI 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.
--Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake
EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Attachment
e
and a variable error
so for sake of clarity I would love to see variable names that we can easily readSumming 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
Hi,--Harshal DhumalSr. Software EngineerOn Tue, Feb 20, 2018 at 10:34 PM, Dave Page <dpage@pgadmin.org> wrote:HiOn Tue, Feb 20, 2018 at 7:22 AM, Harshal Dhumal <harshal.dhumal@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 oneI 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
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.
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 callede
and a variableerror
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 codeSumming 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 ThanksJoaoOn Wed, Feb 21, 2018 at 4:32 AM Harshal Dhumal <harshal.dhumal@enterprisedb.com> wrote: Hi,--Harshal DhumalSr. Software EngineerOn Tue, Feb 20, 2018 at 10:34 PM, Dave Page <dpage@pgadmin.org> wrote:HiOn Tue, Feb 20, 2018 at 7:22 AM, Harshal Dhumal <harshal.dhumal@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 oneI 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
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake
EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On Wed, Feb 21, 2018 at 4:22 PM, Joao De Almeida Pereira <jdealmeidapereira@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 callede
and a variableerror
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 codeSumming 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
ThanksJoaoOn Wed, Feb 21, 2018 at 4:32 AM Harshal Dhumal <harshal.dhumal@enterprisedb.com> wrote:Hi,--Harshal DhumalSr. Software EngineerOn Tue, Feb 20, 2018 at 10:34 PM, Dave Page <dpage@pgadmin.org> wrote:HiOn Tue, Feb 20, 2018 at 7:22 AM, Harshal Dhumal <harshal.dhumal@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 oneI 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
Yep I installed the V2 fileOn Wed, Feb 21, 2018 at 11:31 AM Dave Page <dpage@pgadmin.org> wrote:On Wed, Feb 21, 2018 at 4:22 PM, Joao De Almeida Pereira <jdealmeidapereira@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.
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).
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 callede
and a variableerror
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 ThanksJoaoOn Wed, Feb 21, 2018 at 4:32 AM Harshal Dhumal <harshal.dhumal@enterprisedb.com> wrote: Hi,--Harshal DhumalSr. Software EngineerOn Tue, Feb 20, 2018 at 10:34 PM, Dave Page <dpage@pgadmin.org> wrote:HiOn Tue, Feb 20, 2018 at 7:22 AM, Harshal Dhumal <harshal.dhumal@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 oneI 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
Hi,On Wed, Feb 21, 2018 at 10:59 PM, Joao De Almeida Pereira <jdealmeidapereira@pivotal.io> wrote:Yep I installed the V2 fileOn Wed, Feb 21, 2018 at 11:31 AM Dave Page <dpage@pgadmin.org> wrote:On Wed, Feb 21, 2018 at 4:22 PM, Joao De Almeida Pereira <jdealmeidapereira@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 behaviorOk I'll move dialogTabNavigator to new file and will add test cases.- There is no difference between a variable callede
and a variableerror
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 uniformOk I'll do this.- I noticed that there are some linting issues in the Javascript codeI 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.isLastTabOf
Child ThanksJoaoOn Wed, Feb 21, 2018 at 4:32 AM Harshal Dhumal <harshal.dhumal@enterprisedb.com> wrote: Hi,--Harshal DhumalSr. Software EngineerOn Tue, Feb 20, 2018 at 10:34 PM, Dave Page <dpage@pgadmin.org> wrote:HiOn Tue, Feb 20, 2018 at 7:22 AM, Harshal Dhumal <harshal.dhumal@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 oneI 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
Attachment
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.
--Harshal DhumalSr. Software EngineerOn Wed, Feb 21, 2018 at 11:39 PM, Harshal Dhumal <harshaldhumal15@gmail.com> wrote:Hi,On Wed, Feb 21, 2018 at 10:59 PM, Joao De Almeida Pereira <jdealmeidapereira@pivotal.io> wrote:Yep I installed the V2 fileOn Wed, Feb 21, 2018 at 11:31 AM Dave Page <dpage@pgadmin.org> wrote:On Wed, Feb 21, 2018 at 4:22 PM, Joao De Almeida Pereira <jdealmeidapereira@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 behaviorOk I'll move dialogTabNavigator to new file and will add test cases.- There is no difference between a variable callede
and a variableerror
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 uniformOk I'll do this.- I noticed that there are some linting issues in the Javascript codeI 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.isLastTabOf
Child ThanksJoaoOn Wed, Feb 21, 2018 at 4:32 AM Harshal Dhumal <harshal.dhumal@enterprisedb.com> wrote: Hi,--Harshal DhumalSr. Software EngineerOn Tue, Feb 20, 2018 at 10:34 PM, Dave Page <dpage@pgadmin.org> wrote:HiOn Tue, Feb 20, 2018 at 7:22 AM, Harshal Dhumal <harshal.dhumal@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 oneI 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
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake
EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Attachment
HiOn Mon, Feb 26, 2018 at 12:03 PM, Harshal Dhumal <harshal.dhumal@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 DhumalSr. Software EngineerOn Wed, Feb 21, 2018 at 11:39 PM, Harshal Dhumal <harshaldhumal15@gmail.com> wrote:Hi,On Wed, Feb 21, 2018 at 10:59 PM, Joao De Almeida Pereira <jdealmeidapereira@pivotal.io> wrote:Yep I installed the V2 fileOn Wed, Feb 21, 2018 at 11:31 AM Dave Page <dpage@pgadmin.org> wrote:On Wed, Feb 21, 2018 at 4:22 PM, Joao De Almeida Pereira <jdealmeidapereira@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 behaviorOk I'll move dialogTabNavigator to new file and will add test cases.- There is no difference between a variable callede
and a variableerror
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 uniformOk I'll do this.- I noticed that there are some linting issues in the Javascript codeI 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.isLastTabOf
Child ThanksJoaoOn Wed, Feb 21, 2018 at 4:32 AM Harshal Dhumal <harshal.dhumal@enterprisedb.com> wrote: Hi,--Harshal DhumalSr. Software EngineerOn Tue, Feb 20, 2018 at 10:34 PM, Dave Page <dpage@pgadmin.org> wrote:HiOn Tue, Feb 20, 2018 at 7:22 AM, Harshal Dhumal <harshal.dhumal@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 oneI 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
Attachment
Hi,Please find the updated patch.On Mon, Feb 26, 2018 at 8:03 PM, Dave Page <dpage@pgadmin.org> wrote:HiOn Mon, Feb 26, 2018 at 12:03 PM, Harshal Dhumal <harshal.dhumal@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 DhumalSr. Software EngineerOn Wed, Feb 21, 2018 at 11:39 PM, Harshal Dhumal <harshaldhumal15@gmail.com> wrote:Hi,On Wed, Feb 21, 2018 at 10:59 PM, Joao De Almeida Pereira <jdealmeidapereira@pivotal.io> wrote:Yep I installed the V2 fileOn Wed, Feb 21, 2018 at 11:31 AM Dave Page <dpage@pgadmin.org> wrote:On Wed, Feb 21, 2018 at 4:22 PM, Joao De Almeida Pereira <jdealmeidapereira@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 behaviorOk I'll move dialogTabNavigator to new file and will add test cases.- There is no difference between a variable callede
and a variableerror
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 uniformOk I'll do this.- I noticed that there are some linting issues in the Javascript codeI 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.isLastTabOf
Child ThanksJoaoOn Wed, Feb 21, 2018 at 4:32 AM Harshal Dhumal <harshal.dhumal@enterprisedb.com> wrote: Hi,--Harshal DhumalSr. Software EngineerOn Tue, Feb 20, 2018 at 10:34 PM, Dave Page <dpage@pgadmin.org> wrote:HiOn Tue, Feb 20, 2018 at 7:22 AM, Harshal Dhumal <harshal.dhumal@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 oneI 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
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake
EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
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.dhumal@enterprisedb.com> wrote: Hi,Please find the updated patch.On Mon, Feb 26, 2018 at 8:03 PM, Dave Page <dpage@pgadmin.org> wrote:HiOn Mon, Feb 26, 2018 at 12:03 PM, Harshal Dhumal <harshal.dhumal@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 DhumalSr. Software EngineerOn Wed, Feb 21, 2018 at 11:39 PM, Harshal Dhumal <harshaldhumal15@gmail.com> wrote:Hi,On Wed, Feb 21, 2018 at 10:59 PM, Joao De Almeida Pereira <jdealmeidapereira@pivotal.io> wrote:Yep I installed the V2 fileOn Wed, Feb 21, 2018 at 11:31 AM Dave Page <dpage@pgadmin.org> wrote:On Wed, Feb 21, 2018 at 4:22 PM, Joao De Almeida Pereira <jdealmeidapereira@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 behaviorOk I'll move dialogTabNavigator to new file and will add test cases.- There is no difference between a variable callede
and a variableerror
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 uniformOk I'll do this.- I noticed that there are some linting issues in the Javascript codeI 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.isLastTabOf
Child ThanksJoaoOn Wed, Feb 21, 2018 at 4:32 AM Harshal Dhumal <harshal.dhumal@enterprisedb.com> wrote: Hi,--Harshal DhumalSr. Software EngineerOn Tue, Feb 20, 2018 at 10:34 PM, Dave Page <dpage@pgadmin.org> wrote:HiOn Tue, Feb 20, 2018 at 7:22 AM, Harshal Dhumal <harshal.dhumal@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 oneI 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
Attachment
Hi,On Tue, Feb 27, 2018 at 1:08 AM, Dave Page <dpage@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 andthis 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.dhumal@enterprisedb.com> wrote: Hi,Please find the updated patch.On Mon, Feb 26, 2018 at 8:03 PM, Dave Page <dpage@pgadmin.org> wrote:HiOn Mon, Feb 26, 2018 at 12:03 PM, Harshal Dhumal <harshal.dhumal@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 DhumalSr. Software EngineerOn Wed, Feb 21, 2018 at 11:39 PM, Harshal Dhumal <harshaldhumal15@gmail.com> wrote:Hi,On Wed, Feb 21, 2018 at 10:59 PM, Joao De Almeida Pereira <jdealmeidapereira@pivotal.io> wrote:Yep I installed the V2 fileOn Wed, Feb 21, 2018 at 11:31 AM Dave Page <dpage@pgadmin.org> wrote:On Wed, Feb 21, 2018 at 4:22 PM, Joao De Almeida Pereira <jdealmeidapereira@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 behaviorOk I'll move dialogTabNavigator to new file and will add test cases.- There is no difference between a variable callede
and a variableerror
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 uniformOk I'll do this.- I noticed that there are some linting issues in the Javascript codeI 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.isLastTabOf
Child ThanksJoaoOn Wed, Feb 21, 2018 at 4:32 AM Harshal Dhumal <harshal.dhumal@enterprisedb.com> wrote: Hi,--Harshal DhumalSr. Software EngineerOn Tue, Feb 20, 2018 at 10:34 PM, Dave Page <dpage@pgadmin.org> wrote:HiOn Tue, Feb 20, 2018 at 7:22 AM, Harshal Dhumal <harshal.dhumal@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 oneI 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
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake
EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company