Re: [pgadmin-hackers][patch] History Detail Pane

Поиск
Список
Период
Сортировка
От Surinder Kumar
Тема Re: [pgadmin-hackers][patch] History Detail Pane
Дата
Msg-id CAM5-9D80zQgOL+Cyq4DNH-MxrY1fHK5j_APoWyJ-bke5qkyWPQ@mail.gmail.com
обсуждение исходный текст
Ответ на Re: [pgadmin-hackers][patch] History Detail Pane  (George Gelashvili <ggelashvili@pivotal.io>)
Ответы Re: [pgadmin-hackers][patch] History Detail Pane  (Joao Pedro De Almeida Pereira <jdealmeidapereira@pivotal.io>)
Список pgadmin-hackers
Дерево обсуждения
Hi
On Fri, Jun 23, 2017 at 11:39 PM, George Gelashvili <ggelashvili@pivotal.io> wrote:
On Fri, Jun 23, 2017 at 11:24 AM, Surinder Kumar <surinder.kumar@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.

​2. CSS property flex is supported for IE >= 9 as per reference
​. 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.​

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 ?​

​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

В списке pgadmin-hackers по дате отправления:

Предыдущее
От: Khushboo Vashi
Дата:
Сообщение: [pgAdmin4][patch]: Fixed RM #2507 : REVOKE privileges do not displayin SQL tab for the function
Следующее
От: Harshal Dhumal
Дата:
Сообщение: Re: [pgadmin-hackers] Re: Server side cursor limitations for ondemand loading of data in query tool [RM2137] [pgAdmin4]