Обсуждение: [pgadmin-hackers][patch] History Detail Pane
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
Shruti & Sarah
Вложения
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@pivotal.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@postgresql.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
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@pgadmin.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@pivotal.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@postgresql.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@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgadmin-hackers
Sounds good! We're looking into it.
On Wed, Jun 14, 2017 at 12:15 PM, Robert Eckhardt <reckhardt@pivotal.io> wrote:
Absolutely makes sense.Matt, Sarah,Do we understand the issues Dave is mentioning well enough to look into it and tackle it?-- RobOn Wed, Jun 14, 2017 at 8:12 AM, Dave Page <dpage@pgadmin.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@pivotal.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@postgresql.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@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgadmin-hackers
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@pivotal.io> wrote:
Sounds good! We're looking into it.On Wed, Jun 14, 2017 at 12:15 PM, Robert Eckhardt <reckhardt@pivotal.io> wrote:Absolutely makes sense.Matt, Sarah,Do we understand the issues Dave is mentioning well enough to look into it and tackle it?-- RobOn Wed, Jun 14, 2017 at 8:12 AM, Dave Page <dpage@pgadmin.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@pivotal.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@postgresql.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@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgadmin-hackers
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@pivotal.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@pivotal.io> wrote: >> >> Sounds good! We're looking into it. >> >> On Wed, Jun 14, 2017 at 12:15 PM, Robert Eckhardt <reckhardt@pivotal.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@pgadmin.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@pivotal.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@postgresql.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@postgresql.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
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@pgadmin.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@pivotal.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@pivotal.io> wrote:
>>
>> Sounds good! We're looking into it.
>>
>> On Wed, Jun 14, 2017 at 12:15 PM, Robert Eckhardt <reckhardt@pivotal.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@pgadmin.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@pivotal.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@postgresql.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@postgresql.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@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgadmin-hackers
Вложения
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@pivotal.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@pgadmin.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@pivotal.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@pivotal.io> >> > wrote: >> >> >> >> Sounds good! We're looking into it. >> >> >> >> On Wed, Jun 14, 2017 at 12:15 PM, Robert Eckhardt >> >> <reckhardt@pivotal.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@pgadmin.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@pivotal.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@postgresql.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@postgresql.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@postgresql.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
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@pgadmin.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@pivotal.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@pgadmin.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@pivotal.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@pivotal.io>
>> > wrote:
>> >>
>> >> Sounds good! We're looking into it.
>> >>
>> >> On Wed, Jun 14, 2017 at 12:15 PM, Robert Eckhardt
>> >> <reckhardt@pivotal.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@pgadmin.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@pivotal.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@postgresql.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@postgresql.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@postgresql.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
Вложения
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. 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@pivotal.io> wrote:
Hi HackersYou 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ãoOn Wed, Jun 21, 2017 at 12:22 PM, Dave Page <dpage@pgadmin.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@pivotal.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@pgadmin.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@pivotal.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@pivotal.io>
>> > wrote:
>> >>
>> >> Sounds good! We're looking into it.
>> >>
>> >> On Wed, Jun 14, 2017 at 12:15 PM, Robert Eckhardt
>> >> <reckhardt@pivotal.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@pgadmin.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@pivotal.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@postgresql.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@postgresql.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@postgresql.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
On Fri, Jun 23, 2017 at 11:24 AM, Surinder Kumar <surinder.kumar@enterprisedb.com> wrote:
HiReview 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. 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@pivotal.io> wrote:Hi HackersYou 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ãoOn Wed, Jun 21, 2017 at 12:22 PM, Dave Page <dpage@pgadmin.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@pivotal.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@pgadmin.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@pivotal.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@pivotal.io>
>> > wrote:
>> >>
>> >> Sounds good! We're looking into it.
>> >>
>> >> On Wed, Jun 14, 2017 at 12:15 PM, Robert Eckhardt
>> >> <reckhardt@pivotal.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@pgadmin.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@pivotal.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@postgresql.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@postgresql.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@postgresql.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
On Fri, Jun 23, 2017 at 11:24 AM, Surinder Kumar <surinder.kumar@enterprisedb. com> wrote:
HiReview 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?
Anyways IE-9/10 are outdated and no longer supported by Microsoft, the only supported browsers are IE-11+.2. CSS property flex is supported for IE >= 9 as per reference. I tested this patch on IE and layout is broken.
Does this patch work in supported IEs?
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.
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
Вложения
Hi
On Fri, Jun 23, 2017 at 11:39 PM, George Gelashvili <ggelashvili@pivotal.io> wrote:
On Fri, Jun 23, 2017 at 11:24 AM, Surinder Kumar <surinder.kumar@enterprisedb.com> wrote: HiReview 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.
Anyways IE-9/10 are outdated and no longer supported by Microsoft, the only supported browsers are IE-11+.2. CSS property flex is supported for IE >= 9 as per reference. I tested this patch on IE and layout is broken.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.
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 ?
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.ThanksShruti and George
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.kumar@enterprisedb.com> wrote:
HiOn Fri, Jun 23, 2017 at 11:39 PM, George Gelashvili <ggelashvili@pivotal.io> wrote:On Fri, Jun 23, 2017 at 11:24 AM, Surinder Kumar <surinder.kumar@enterprisedb.com> wrote: HiReview 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.
Anyways IE-9/10 are outdated and no longer supported by Microsoft, the only supported browsers are IE-11+.2. CSS property flex is supported for IE >= 9 as per reference. I tested this patch on IE and layout is broken.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 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.ThanksShruti and George
Thanks,
Joao and Shruti
Вложения
On Mon, Jun 26, 2017 at 10:08 AM, Joao Pedro De Almeida Pereira <jdealmeidapereira@pivotal.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.kumar@enterprisedb.com> wrote:
>>
>> Hi
>> On Fri, Jun 23, 2017 at 11:39 PM, George Gelashvili
>> <ggelashvili@pivotal.io> wrote:
>>>
>>> On Fri, Jun 23, 2017 at 11:24 AM, Surinder Kumar
>>> <surinder.kumar@enterprisedb.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
>>>>> . 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 to try to support all the existing browser. Nevertheless we
> believe that this should be done in a future patch.
Here are the IE stats from www.pgadmin.org:
Browser Version | Sessions | % |
11.0 | 1233 | 92.85% |
8.0 | 32 | 2.41% |
10.0 | 29 | 2.18% |
9.0 | 23 | 1.73% |
7.0 | 10 | 0.75% |
5.0 | 1 | 0.08% |
1328 | 100.00% |
I think it's fairly safe to ignore IE <= 10.0
>>>> 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
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@pivotal.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.kumar@enterprisedb.com> wrote: HiOn Fri, Jun 23, 2017 at 11:39 PM, George Gelashvili <ggelashvili@pivotal.io> wrote:On Fri, Jun 23, 2017 at 11:24 AM, Surinder Kumar <surinder.kumar@enterprisedb.com> wrote: HiReview 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.Anyways IE-9/10 are outdated and no longer supported by Microsoft, the only supported browsers are IE-11+.2. CSS property flex is supported for IE >= 9 as per reference. I tested this patch on IE and layout is broken.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 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.ThanksShruti and GeorgeThanks,Joao and Shruti
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake
EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake
EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Yep,
please see attached
On Tue, Jun 27, 2017 at 9:11 AM, Dave Page <dpage@pgadmin.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@pivotal.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.kumar@enterprisedb.com> wrote: HiOn Fri, Jun 23, 2017 at 11:39 PM, George Gelashvili <ggelashvili@pivotal.io> wrote:On Fri, Jun 23, 2017 at 11:24 AM, Surinder Kumar <surinder.kumar@enterprisedb.com> wrote: HiReview 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.Anyways IE-9/10 are outdated and no longer supported by Microsoft, the only supported browsers are IE-11+.2. CSS property flex is supported for IE >= 9 as per reference. I tested this patch on IE and layout is broken.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 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.ThanksShruti and GeorgeThanks,Joao and ShrutiDave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake
EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Вложения
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@pivotal.io> wrote:
Yep,please see attachedOn Tue, Jun 27, 2017 at 9:11 AM, Dave Page <dpage@pgadmin.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@pivotal.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.kumar@enterprisedb.com> wrote: HiOn Fri, Jun 23, 2017 at 11:39 PM, George Gelashvili <ggelashvili@pivotal.io> wrote:On Fri, Jun 23, 2017 at 11:24 AM, Surinder Kumar <surinder.kumar@enterprisedb.com> wrote: HiReview 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.Anyways IE-9/10 are outdated and no longer supported by Microsoft, the only supported browsers are IE-11+.2. CSS property flex is supported for IE >= 9 as per reference. I tested this patch on IE and layout is broken.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 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.ThanksShruti and GeorgeThanks,Joao and ShrutiDave 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
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake
EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Hi,
New history pane is really nice and informative than existing one.
I'm just thinking about one minor improvement we can make, to allow history tab to respond to up and down arrow keys so that user can easily switch between executed queries without using mouse.
Thanks,
--
Harshal Dhumal
Sr. Software Engineer
On Tue, Jun 27, 2017 at 8:26 PM, Dave Page <dpage@pgadmin.org> wrote:
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@pivotal.io> wrote:Yep,please see attachedOn Tue, Jun 27, 2017 at 9:11 AM, Dave Page <dpage@pgadmin.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@pivotal.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.kumar@enterprisedb.com> wrote: HiOn Fri, Jun 23, 2017 at 11:39 PM, George Gelashvili <ggelashvili@pivotal.io> wrote:On Fri, Jun 23, 2017 at 11:24 AM, Surinder Kumar <surinder.kumar@enterprisedb.com> wrote: HiReview 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.Anyways IE-9/10 are outdated and no longer supported by Microsoft, the only supported browsers are IE-11+.2. CSS property flex is supported for IE >= 9 as per reference. I tested this patch on IE and layout is broken.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 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.ThanksShruti and GeorgeThanks,Joao and ShrutiDave 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
Harshal,
Thanks for taking a look. That exact feature should be in our next patch along with a few style changes.
-- Rob
On Tue, Jun 27, 2017 at 2:03 PM, Harshal Dhumal <harshal.dhumal@enterprisedb.com> wrote:
Hi,New history pane is really nice and informative than existing one.I'm just thinking about one minor improvement we can make, to allow history tab to respond to up and down arrow keys so that user can easily switch between executed queries without using mouse.Thanks,--Harshal DhumalSr. Software EngineerOn Tue, Jun 27, 2017 at 8:26 PM, Dave Page <dpage@pgadmin.org> wrote: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@pivotal.io> wrote:Yep,please see attachedOn Tue, Jun 27, 2017 at 9:11 AM, Dave Page <dpage@pgadmin.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@pivotal.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.kumar@enterprisedb.com> wrote: HiOn Fri, Jun 23, 2017 at 11:39 PM, George Gelashvili <ggelashvili@pivotal.io> wrote:On Fri, Jun 23, 2017 at 11:24 AM, Surinder Kumar <surinder.kumar@enterprisedb.com> wrote: HiReview 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.Anyways IE-9/10 are outdated and no longer supported by Microsoft, the only supported browsers are IE-11+.2. CSS property flex is supported for IE >= 9 as per reference. I tested this patch on IE and layout is broken.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 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.ThanksShruti and GeorgeThanks,Joao and ShrutiDave 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
On Tue, Jun 27, 2017 at 8:26 PM, Dave Page <dpage@pgadmin.org> wrote:
Thanks - patch applied (just in time for Raffi's & my talk)!
The history tab looks really very good.
On Tue, Jun 27, 2017 at 10:05 AM, Joao Pedro De Almeida Pereira <jdealmeidapereira@pivotal.io> wrote:Yep,please see attachedOn Tue, Jun 27, 2017 at 9:11 AM, Dave Page <dpage@pgadmin.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@pivotal.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.kumar@enterprisedb.com> wrote: HiOn Fri, Jun 23, 2017 at 11:39 PM, George Gelashvili <ggelashvili@pivotal.io> wrote:On Fri, Jun 23, 2017 at 11:24 AM, Surinder Kumar <surinder.kumar@enterprisedb.com> wrote: HiReview 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.Anyways IE-9/10 are outdated and no longer supported by Microsoft, the only supported browsers are IE-11+.2. CSS property flex is supported for IE >= 9 as per reference. I tested this patch on IE and layout is broken.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 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.ThanksShruti and GeorgeThanks,Joao and ShrutiDave 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