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

Thanks guys - patch applied.

On Mon, Feb 26, 2018 at 5:31 PM, Murtuza Zabuawala <
murtuza(dot)zabuawala(at)enterprisedb(dot)com> wrote:

> Thank you Joao for reviewing.
>
> On Mon, Feb 26, 2018 at 10:43 PM, Joao De Almeida Pereira <
> jdealmeidapereira(at)pivotal(dot)io> wrote:
>
>> Hello Murtuza,
>> We just passed the patch through our CI pipeline and all tests pass. The
>> patch looks good.
>>
>> Sorry that the patch I sent didn't help. Nevertheless when I finish the
>> tasks that we want to try to get accepted for pgAdmin4 3.0 release I will
>> try to change the way the preferences are retrieved, because at this point
>> in time they work because we only ask for them after some time we try to
>> get them, or else the code does not work.
>> Unless someone want to give it a shot first. From my point of view this
>> functionality should work like a separate service that we include where we
>> need it and work with a promise system. There are some nice article about
>> promises and how to handle them instead of using event bouncing all around:
>> https://medium.com/dev-bits/writing-neat-asynchronous-node-
>> js-code-with-promises-32ed3a4fd098
>> https://developers.google.com/web/fundamentals/primers/promises (this is
>> quite a complete guide on promises, and how to think about them)
>>
>> Sure, I'll go through the articles you provided.
>
> Thanks,
> Murtuza
>
>
>> Thanks
>> Joao
>>
>> On Mon, Feb 26, 2018 at 4:58 AM Murtuza Zabuawala <
>> murtuza(dot)zabuawala(at)enterprisedb(dot)com> wrote:
>>
>>> Hi Joao,
>>>
>>> I tried the solution/patch you suggested but it is not working for me, I
>>> always get *undefined* for preferences values.
>>>
>>> Thanks,
>>> Murtuza
>>>
>>> On Thu, Feb 22, 2018 at 3:17 AM, Joao De Almeida Pereira <
>>> jdealmeidapereira(at)pivotal(dot)io> wrote:
>>>
>>>> Hello Murtuza,
>>>> I created a small patch on the way I believe the function should work.
>>>> Not 100% sure why we are piggybacking on the window.top, window.opener and
>>>> so on to give us information. Nowadays I do not think that loading the JS
>>>> files again costs that much in terms of time.
>>>>
>>>> The issue I came across was that cache_preferences/get_preference
>>>> behave in a very strange way.
>>>>
>>>> Example:
>>>> If the cache is populated no problem it returns a value and it is fine
>>>> If the cache is not populated it sets a timeout that will check in 1
>>>> second if is the cache is populated if it is not then does nothing, if it
>>>> is then returns the value.
>>>> The problem is that it returns the value to no place.
>>>>
>>>> This caching implementation works until this point because for some
>>>> good fortune we never call get_preference before we have the cache full.
>>>> Is my assessment correct?
>>>>
>>>> Nevertheless if the caching was using promises we could avoid the
>>>> problem above.
>>>>
>>>> Thanks
>>>> Joao
>>>>
>>>> On Wed, Feb 21, 2018 at 12:50 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