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 < jdealmeidapere...@pivotal.io> wrote:
> Yep, > please see attached > > On Tue, Jun 27, 2017 at 9:11 AM, Dave Page <dp...@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 < >> 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 >> > > -- Dave Page Blog: http://pgsnake.blogspot.com Twitter: @pgsnake EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company
