Re: [pgAdmin4][RM#2900] Adding accessibility features in query tool

Hi,

PFA updated patch.

--
Regards,
Murtuza Zabuawala
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Wed, Feb 21, 2018 at 11:19 PM, Murtuza Zabuawala <
murtuza(dot)zabuawala(at)enterprisedb(dot)com> wrote:

> Hi Joao,
>
> Thank you for your time in reviewing the patch.
>
>
> On Wed, Feb 21, 2018 at 9:11 PM, Joao De Almeida Pereira <
> jdealmeidapereira(at)pivotal(dot)io> wrote:
>
>> Hello Murtuza,
>> After reviewing the code I have some suggestions:
>> - We should split the PEP and the features into different patches, or
>> else it becomes very hard to separate Feature code from Code Style change
>>
> ​I'll keep that in mind next time onwards.
>
>> - The function: `getTextRepresentaionShortcut` has a typo
>>
> ​I'll fix​
>
> ​that.​
>
>> - As a personal point I have a hard time reading multiple declarations
>> of variables in javascript under a single `var`, I prefer 1 `let` per
>> variable
>>
> - I think we should use cameCase for our variables in Javascript code
>> - What is the reason behind the move of the key shortcuts back into the
>> SQLEditorController? This look like something that could be extracted from
>> SQLEditor file into its own
>>
> I'll try to move that code.​
>
>
>> - Looks like we are missing some test coverage on the new implemented
>> parts
>>
> ​I didn't added any new code as such, I just moved out preferences code
> out from main file.
>
>> - The getKeyboardShortcuts function is retrieving information from
>> window.top and window.opener I think that we can come up with a better
>> pattern then this. Relying on window information doesn't look very good.
>> That was the pattern for JQuery that the new frameworks are trying to avoid
>> because polluting the global scope is almost always a Code Smell.
>>
> ​What do you suggest on this?​
>
>
>>
>>
>> I see there was some refactoring on the backend with the creation of
>> RegisterQueryToolPreferences and it is looking good, hope that we can do
>> this in more places specially in the front-end.
>>
>> Using the patch that you sent I passed it through our CI and everything
>> is Passing.
>>
>> Thanks
>> Joao
>>
>> On Tue, Feb 20, 2018 at 1:13 PM Murtuza Zabuawala <
>> murtuza(dot)zabuawala(at)enterprisedb(dot)com> wrote:
>>
>>> Hi,
>>>
>>> PFA patch for adding accessibility features in query tool.
>>> RM#2900
>>>
>>> Please review.
>>>
>>> --
>>> Regards,
>>> Murtuza Zabuawala
>>> EnterpriseDB: http://www.enterprisedb.com
>>> The Enterprise PostgreSQL Company
>>>
>>>
>