Re: [pgadmin-hackers][patch] History Detail Pane

Hi,

New history pane is really nice and informative than existing one.
I'm just thinking about one minor improvement we can make, to allow history
tab to respond to up and down arrow keys so that user can easily switch
between executed queries without using mouse.

Thanks,

--
*Harshal Dhumal*
*Sr. Software Engineer*

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

On Tue, Jun 27, 2017 at 8:26 PM, Dave Page <dpage(at)pgadmin(dot)org> wrote:

> Thanks - patch applied (just in time for Raffi's & my talk)!
>
> On Tue, Jun 27, 2017 at 10:05 AM, Joao Pedro De Almeida Pereira <
> jdealmeidapereira(at)pivotal(dot)io> wrote:
>
>> Yep,
>> please see attached
>>
>> On Tue, Jun 27, 2017 at 9:11 AM, Dave Page <dpage(at)pgadmin(dot)org> wrote:
>>
>>> Can you rebase this patch please?
>>>
>>> Thankfully I think we're basically at the end of our changes in this
>>> area!
>>>
>>> On Mon, Jun 26, 2017 at 10:08 AM, Joao Pedro De Almeida Pereira <
>>> jdealmeidapereira(at)pivotal(dot)io> wrote:
>>>
>>>> Hi Surinder,
>>>>
>>>> You can find our answers inline and attached the patch with the change
>>>> of scrolls from "scroll" to "auto"
>>>>
>>>> On Mon, Jun 26, 2017 at 4:47 AM, Surinder Kumar <
>>>> surinder(dot)kumar(at)enterprisedb(dot)com> wrote:
>>>>
>>>>> Hi
>>>>> On Fri, Jun 23, 2017 at 11:39 PM, George Gelashvili <
>>>>> ggelashvili(at)pivotal(dot)io> wrote:
>>>>>
>>>>>> On Fri, Jun 23, 2017 at 11:24 AM, Surinder Kumar <
>>>>>> surinder(dot)kumar(at)enterprisedb(dot)com> wrote:
>>>>>>
>>>>>>> Hi
>>>>>>>
>>>>>>> Review comments:
>>>>>>>
>>>>>>> ​1. ​
>>>>>>> Can we set "message"(in message detail) style property scroll to
>>>>>>> ​'​
>>>>>>> auto
>>>>>>> ​'​
>>>>>>> instead of
>>>>>>> ​'​
>>>>>>> scroll
>>>>>>> ​'​
>>>>>>> ?
>>>>>>>
>>>>>>
>>>>>> Could you elaborate why 'auto' is what we want?
>>>>>>
>>>>> ​The scroll bars should appear only when content is beyond the
>>>>> width/height of container.​ Now with 'scroll', the border layout around
>>>>> container appears irrespective of content width/height. If we set 'auto',
>>>>> it won't appear.
>>>>>
>>>>
>>>> We changed that in the attached patch. The scroll bars will now appear
>>>> when the text exceeds the window.
>>>>
>>>>
>>>>>> ​2. CSS property flex is supported for IE >= 9 as per reference
>>>>>>>> <https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_Flexible_Box_Layout/Using_CSS_flexible_boxes>
>>>>>>>> ​. I tested this patch on IE and layout is broken.​
>>>>>>>>
>>>>>>>> Anyways IE-9/10 are outdated and no longer supported by
>>>>>>> Microsoft, the only supported browsers are IE-11+.
>>>>>>>
>>>>>>
>>>>>> Does this patch work in supported IEs?
>>>>>>
>>>>> ​I use Windows 7 which has IE9,10, but if we can fix it for IE9,10 we
>>>>> should fix. Majority of people are still using IE9,10.​
>>>>>
>>>>
>>>> We based almost 100% of this patch on Flexbox. In order to use another
>>>> structure, we would have to rewrite the html blocks (the majority of the
>>>> UI). If the community believes that is important to keep giving support to
>>>> Browsers that are no longer supported then we can use a library like
>>>> modernizr <https://modernizr.com> to try to support all the existing
>>>> browser. Nevertheless we believe that this should be done in a future patch.
>>>>
>>>>
>>>>>> 3. ​Can the CSS styles in ‘history_detail_message.jsx’ moved out in
>>>>>>> a separate stylesheet file
>>>>>>> ​ as inline styles in html are never recommended.​
>>>>>>>
>>>>>>
>>>>>> We've been trying to use inline styles rather than classes for our
>>>>>> react jsx code, as this keeps element behavior declarative. This includes
>>>>>> both functionality and appearance.
>>>>>> So far the pattern has been to extract styles used by more than one
>>>>>> component to jsx/styles.
>>>>>>
>>>>> ​Can we use classes and then write css around classes thus preserving
>>>>> element behaviour declarative ?​
>>>>>
>>>>
>>>> We believe that this should be a conversation to have next Friday in
>>>> the Hackers Community Forum. In the meanwhile we hope this is not a blocker
>>>> to the merge of this patch.
>>>>
>>>>
>>>>>
>>>>>> ​4. In 'codemirror.jsx', setInterval is used to bind
>>>>>>> hydrateWhenBecomesVisible function after every 100ms. Can setTimeout ​be
>>>>>>> used as it needs to bind only once ?
>>>>>>> Also if setInterval is used, in componentWillUnmount
>>>>>>> clearInterval(...) should be implemented.
>>>>>>>
>>>>>>
>>>>>> We actually need to poll, as otherwise the codemirror element will
>>>>>> render with its last state (so, incorrect query history)
>>>>>>
>>>>>> 5. The text like 'Rows Affected' or 'Duration' should be wrapped in
>>>>>>> 'gettext' for translation?
>>>>>>
>>>>>>
>>>>>> We are working on using translations in React components. This needs
>>>>>> additional effort and we'll send this in a separate patch.
>>>>>>
>>>>>> Thanks
>>>>>> Shruti and George
>>>>>>
>>>>>
>>>>>
>>>> Thanks,
>>>> Joao and Shruti
>>>>
>>>
>>>
>>>
>>> --
>>> 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
>