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

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