Harshal, Thanks for taking a look. That exact feature should be in our next patch along with a few style changes.
-- Rob On Tue, Jun 27, 2017 at 2:03 PM, Harshal Dhumal < harshal.dhu...@enterprisedb.com> wrote: > Hi, > > New history pane is really nice and informative than existing one. > I'm just thinking about one minor improvement we can make, to allow > history tab to respond to up and down arrow keys so that user can easily > switch between executed queries without using mouse. > > > Thanks, > > -- > *Harshal Dhumal* > *Sr. Software Engineer* > > EnterpriseDB India: http://www.enterprisedb.com > The Enterprise PostgreSQL Company > > 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)! >> >> 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 >> > >
