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 < 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 >>>>> <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+. >>>> >>> >>> 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 <https://modernizr.com> 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. >>> >>> 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
