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

FYI, in an attempt not to be the one reviewing everything, I'm leaving this
for Joao to pick up again. Moving forwards, I'd like to make that the
default - if (better yet, when) someone reviews someone else's patch, we
should expect that person to review any followups as well, unless they
explicitly say otherwise. In such cases, those of us with commit-bits
should hold back until the initial reviewer is happy.

Thanks :-)

On Mon, Feb 26, 2018 at 9:40 AM, Murtuza Zabuawala <
murtuza(dot)zabuawala(at)enterprisedb(dot)com> wrote:

> 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
>>>>
>>>>
>>
>

--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company