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

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)enterpri
> sedb.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.

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

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

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