On Mon, Jun 26, 2017 at 10:08 AM, Joao Pedro De Almeida Pereira < jdealmeidapere...@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.ku...@enterprisedb.com> wrote: >> >> Hi >> On Fri, Jun 23, 2017 at 11:39 PM, George Gelashvili >> <ggelashv...@pivotal.io> wrote: >>> >>> 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 >>>> ' >>>> ? >>> >>> >>> 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
