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

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
> ​'​
> ?
>
> ​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+.

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