From: | George Gelashvili <ggelashvili(at)pivotal(dot)io> |
---|---|
To: | Murtuza Zabuawala <murtuza(dot)zabuawala(at)enterprisedb(dot)com> |
Cc: | Surinder Kumar <surinder(dot)kumar(at)enterprisedb(dot)com>, Joao Pedro De Almeida Pereira <jdealmeidapereira(at)pivotal(dot)io>, Dave Page <dpage(at)pgadmin(dot)org>, Sarah McAlear <smcalear(at)pivotal(dot)io>, Robert Eckhardt <reckhardt(at)pivotal(dot)io>, pgadmin-hackers <pgadmin-hackers(at)postgresql(dot)org>, Shruti B Iyer <siyer(at)pivotal(dot)io> |
Subject: | Re: [pgadmin-hackers][patch] History Detail Pane |
Date: | 2017-06-23 18:09:15 |
Message-ID: | CAHowoHY0f+vG-0OWtvynAX+kqBgCaygmt0F0b-6wp6A0Lb_PPw@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgadmin-hackers |
On Fri, Jun 23, 2017 at 11:24 AM, Surinder Kumar <
surinder(dot)kumar(at)enterprisedb(dot)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?
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?
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.
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
Attachment | Content-Type | Size |
---|---|---|
create-history-detail-pane.diff | text/plain | 874.0 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Dave Page | 2017-06-25 13:02:11 | Re: [pgAdmin4][Patch][RM_2191] : Add support for the hostaddr connection parameter |
Previous Message | Atul Sharma | 2017-06-23 16:01:42 | Re: [pgAdmin4][Patch][RM_2191] : Add support for the hostaddr connection parameter |