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

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