On Tue, Jun 27, 2017 at 8:26 PM, Dave Page <dp...@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 < > 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 >
