On Fri, Jun 23, 2017 at 11:24 AM, Surinder Kumar < surinder.ku...@enterprisedb.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 < > jdealmeidapere...@pivotal.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 <dp...@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 >>> <ggelashv...@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 <dp...@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 <smcal...@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 <smcal...@pivotal.io >>> > >>> >> > wrote: >>> >> >> >>> >> >> Sounds good! We're looking into it. >>> >> >> >>> >> >> On Wed, Jun 14, 2017 at 12:15 PM, Robert Eckhardt >>> >> >> <reckha...@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 <dp...@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 < >>> smcal...@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-hack...@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-hack...@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-hack...@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 >>> >>> >> >
