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

Hi

Review comments:

​1. ​
Can we set "message"(in message detail) style property scroll to
​'​
auto
​'​
instead of
​'​
scroll
​'​
?

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

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

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

5. The text like 'Rows Affected' or 'Duration' should be wrapped in
'gettext' for translation?

On Thu, Jun 22, 2017 at 11:39 PM, Joao Pedro De Almeida Pereira <
jdealmeidapereira(at)pivotal(dot)io> wrote:

> Hi Hackers
>
> You can find attached a new version of this patch that corrects the issue
> Dave mentioned. We've added direct unit tests for the CodeMirror react
> wrapper to help prevent this bug from creeping in again. See
> ./web/regression/javascript/code_mirror_spec.jsx
>
> Thanks,
> Matt and João
>
> On Wed, Jun 21, 2017 at 12:22 PM, Dave Page <dpage(at)pgadmin(dot)org> wrote:
>
>> Hi
>>
>> Looking good. The only issue I found so far occurs if you open the
>> query tool, then immediately click on the history tab:
>>
>> TypeError: Cannot read property 'CodeMirror' of undefined
>> at Object.<anonymous> (sqleditor.js:882)
>> at triggerEvents (backbone.js:208)
>> at Object.trigger (backbone.js:148)
>> at i.eventFunc (panel.js:101)
>> at i.__trigger (wcDocker.js:1941)
>> at i.__update (wcDocker.js:1818)
>> at i.__onTabChange (wcDocker.js:3970)
>> at i.__update (wcDocker.js:3482)
>> at i.__update (wcDocker.js:2787)
>> at wcDocker.js:20117
>>
>> On Wed, Jun 21, 2017 at 4:45 PM, George Gelashvili
>> <ggelashvili(at)pivotal(dot)io> wrote:
>> > Hi
>> >
>> > We rebased this on top of latest master.
>> >
>> > Cheers,
>> > Matt and George
>> >
>> > On Thu, Jun 15, 2017 at 10:43 AM, Dave Page <dpage(at)pgadmin(dot)org> wrote:
>> >>
>> >> Hi
>> >>
>> >> We use Qt 5.8 at the moment, with the updated QtWebKit TP5 release
>> >> from https://github.com/annulen/webkit/releases. The issue occurs on
>> >> Windows and Mac, and probably Linux as well.
>> >>
>> >> Test builds can be found here: https://developer.pgadmin.org/
>> ~dpage/debug/
>> >>
>> >> On Thu, Jun 15, 2017 at 2:33 PM, Sarah McAlear <smcalear(at)pivotal(dot)io>
>> >> wrote:
>> >> > Hi Dave!
>> >> >
>> >> > Just to verify, which version of QT are you using? The Readme calls
>> for
>> >> > 5.5
>> >> > at the newest. Could you send us the compiled version of the app? Are
>> >> > you
>> >> > only seeing this on Windows?
>> >> >
>> >> > Thanks,
>> >> > Sarah & Shruti & João
>> >> >
>> >> > On Wed, Jun 14, 2017 at 3:46 PM, Sarah McAlear <smcalear(at)pivotal(dot)io>
>> >> > wrote:
>> >> >>
>> >> >> Sounds good! We're looking into it.
>> >> >>
>> >> >> On Wed, Jun 14, 2017 at 12:15 PM, Robert Eckhardt
>> >> >> <reckhardt(at)pivotal(dot)io>
>> >> >> wrote:
>> >> >>>
>> >> >>> Absolutely makes sense.
>> >> >>>
>> >> >>> Matt, Sarah,
>> >> >>>
>> >> >>> Do we understand the issues Dave is mentioning well enough to look
>> >> >>> into
>> >> >>> it and tackle it?
>> >> >>>
>> >> >>> -- Rob
>> >> >>>
>> >> >>> On Wed, Jun 14, 2017 at 8:12 AM, Dave Page <dpage(at)pgadmin(dot)org>
>> wrote:
>> >> >>>>
>> >> >>>> Hi,
>> >> >>>>
>> >> >>>> Before I commit anything else for this patch, we need to fix the
>> >> >>>> existing changes so they work in the desktop runtime (see the
>> >> >>>> previous
>> >> >>>> thread with the patches for the history pane changes). Have any of
>> >> >>>> your team been able to look at that yet?
>> >> >>>>
>> >> >>>> On Wed, Jun 14, 2017 at 4:07 PM, Sarah McAlear <
>> smcalear(at)pivotal(dot)io>
>> >> >>>> wrote:
>> >> >>>> > Hi Hackers!
>> >> >>>> >
>> >> >>>> > This patch includes a new pane in the history tab that displays
>> the
>> >> >>>> > full
>> >> >>>> > formatted query and meta data about the query.
>> >> >>>> >
>> >> >>>> > Thanks!
>> >> >>>> > Shruti & Sarah
>> >> >>>> >
>> >> >>>> >
>> >> >>>> >
>> >> >>>> >
>> >> >>>> >
>> >> >>>> >
>> >> >>>> > --
>> >> >>>> > Sent via pgadmin-hackers mailing list
>> >> >>>> > (pgadmin-hackers(at)postgresql(dot)org)
>> >> >>>> > To make changes to your subscription:
>> >> >>>> > http://www.postgresql.org/mailpref/pgadmin-hackers
>> >> >>>> >
>> >> >>>>
>> >> >>>>
>> >> >>>>
>> >> >>>> --
>> >> >>>> Dave Page
>> >> >>>> Blog: http://pgsnake.blogspot.com
>> >> >>>> Twitter: @pgsnake
>> >> >>>>
>> >> >>>> EnterpriseDB UK: http://www.enterprisedb.com
>> >> >>>> The Enterprise PostgreSQL Company
>> >> >>>>
>> >> >>>>
>> >> >>>> --
>> >> >>>> Sent via pgadmin-hackers mailing list
>> >> >>>> (pgadmin-hackers(at)postgresql(dot)org)
>> >> >>>> To make changes to your subscription:
>> >> >>>> http://www.postgresql.org/mailpref/pgadmin-hackers
>> >> >>>
>> >> >>>
>> >> >>
>> >> >
>> >>
>> >>
>> >>
>> >> --
>> >> Dave Page
>> >> Blog: http://pgsnake.blogspot.com
>> >> Twitter: @pgsnake
>> >>
>> >> EnterpriseDB UK: http://www.enterprisedb.com
>> >> The Enterprise PostgreSQL Company
>> >>
>> >>
>> >> --
>> >> Sent via pgadmin-hackers mailing list (pgadmin-hackers(at)postgresql(dot)org)
>> >> To make changes to your subscription:
>> >> http://www.postgresql.org/mailpref/pgadmin-hackers
>> >
>> >
>>
>>
>>
>> --
>> Dave Page
>> Blog: http://pgsnake.blogspot.com
>> Twitter: @pgsnake
>>
>> EnterpriseDB UK: http://www.enterprisedb.com
>> The Enterprise PostgreSQL Company
>>
>>
>