Re: [pgadmin-hackers][patch] History Detail Pane
От | Dave Page |
---|---|
Тема | Re: [pgadmin-hackers][patch] History Detail Pane |
Дата | |
Msg-id | CA+OCxoyEoLbdDRY1-qGpb9uHOY6_ERA3J3+O1ycLo9VcJ8zz_w@mail.gmail.com обсуждение исходный текст |
Ответ на | Re: [pgadmin-hackers][patch] History Detail Pane (Joao Pedro De Almeida Pereira <jdealmeidapereira@pivotal.io>) |
Список | pgadmin-hackers |
On Mon, Jun 26, 2017 at 10:08 AM, Joao Pedro De Almeida Pereira <jdealmeidapereira@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.kumar@enterprisedb.com> wrote:
>>
>> 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.
>
>
> 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
>>>>> . 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 to try to support all the existing browser. Nevertheless we
> believe that this should be done in a future patch.
Here are the IE stats from www.pgadmin.org:
Browser Version | Sessions | % |
11.0 | 1233 | 92.85% |
8.0 | 32 | 2.41% |
10.0 | 29 | 2.18% |
9.0 | 23 | 1.73% |
7.0 | 10 | 0.75% |
5.0 | 1 | 0.08% |
1328 | 100.00% |
I think it's fairly safe to ignore IE <= 10.0
>>>> 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
В списке pgadmin-hackers по дате отправления:
Предыдущее
От: Dave PageДата:
Сообщение: Re: [pgAdmin4][Patch][RM_2191] : Add support for the hostaddrconnection parameter
Следующее
От: pgAdmin 4 JenkinsДата:
Сообщение: Jenkins build is back to normal : pgadmin4-master-python27 #203