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

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
>