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

Harshal,

Thanks for taking a look. That exact feature should be in our next patch
along with a few style changes.

-- Rob

On Tue, Jun 27, 2017 at 2:03 PM, Harshal Dhumal <
harshal(dot)dhumal(at)enterprisedb(dot)com> wrote:

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