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.kumar@enterpri > sedb.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 >>> <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. > > 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 >
