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

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)!
>
> The history tab looks really very good.

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