Re: [pgAdmin4][patch] update the alert style in the sub-navigation

Thanks patch applied.

On Wed, Aug 16, 2017 at 11:30 AM, Violet Cheng <vcheng(at)pivotal(dot)io> wrote:

> Thanks Surinder! Hope it could be committed soon :)
>
> On Wed, Aug 16, 2017 at 1:34 PM, Surinder Kumar <
> surinder(dot)kumar(at)enterprisedb(dot)com> wrote:
>
>> Hi Violet,
>>
>> I have already reviewed this patch. Here is the link
>> <https://www.postgresql.org/message-id/CAM5-9D8UJVgCD0MrKq6yPx0qx9k8v3r6_4e8SkHdGV1RBDnhbQ%40mail.gmail.com>
>> .
>>
>> Thanks,
>> Surinder
>>
>> On Wed, Aug 16, 2017 at 8:57 AM, Violet Cheng <vcheng(at)pivotal(dot)io> wrote:
>>
>>> Hi,
>>>
>>> Any update on this patch? Could it be committed soon?
>>>
>>> Thanks,
>>> Violet
>>>
>>> On Fri, Aug 11, 2017 at 1:40 PM, Sarah McAlear <smcalear(at)pivotal(dot)io>
>>> wrote:
>>>
>>>> Hi!
>>>>
>>>> We fixed that issue and created a new patch
>>>> <https://www.postgresql.org/message-id/flat/CAGRPzo9wUDtYE2jFz-%3DPbJr%2Btf20zyFncdEoAFdVMkTgoXzf3w%40mail.gmail.com>
>>>> .
>>>>
>>>> Thanks!
>>>>
>>>> On Thu, Aug 10, 2017 at 4:10 PM, Violet Cheng <vcheng(at)pivotal(dot)io>
>>>> wrote:
>>>>
>>>>> Here's the Redmine link
>>>>>
>>>>> https://redmine.postgresql.org/issues/2644
>>>>>
>>>>> On Thu, Aug 10, 2017 at 4:03 PM, Violet Cheng <vcheng(at)pivotal(dot)io>
>>>>> wrote:
>>>>>
>>>>>> Hi Surinder!
>>>>>>
>>>>>> Are you referring to the green message popup? If so, it also appears
>>>>>> to be happening on master. We'll log a bug in our backlog and Redmine and
>>>>>> prioritize it. We agree that it needs to be fixed, but don't think it's
>>>>>> unrelated to this patch.
>>>>>>
>>>>>> Thanks!
>>>>>> Violet & Sarah
>>>>>>
>>>>>> On Thu, Aug 10, 2017 at 2:32 PM, Surinder Kumar <
>>>>>> surinder(dot)kumar(at)enterprisedb(dot)com> wrote:
>>>>>>
>>>>>>> Please find attached screenshot.
>>>>>>>
>>>>>>> On Thu, Aug 10, 2017 at 11:30 AM, Surinder Kumar <
>>>>>>> surinder(dot)kumar(at)enterprisedb(dot)com> wrote:
>>>>>>>
>>>>>>>> Hi Sarah,
>>>>>>>>
>>>>>>>> We noticed that due to this patch, the alert style of "Database
>>>>>>>> connected" message is changed.
>>>>>>>> Can you please look into this?
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Surinder
>>>>>>>>
>>>>>>>> On Wed, Aug 9, 2017 at 4:43 PM, Surinder Kumar <
>>>>>>>> surinder(dot)kumar(at)enterprisedb(dot)com> wrote:
>>>>>>>>
>>>>>>>>> ​Hi,
>>>>>>>>>
>>>>>>>>> ​The updated patch looks good to me.
>>>>>>>>>
>>>>>>>>> On Wed, Aug 9, 2017 at 4:15 PM, Sarah McAlear <smcalear(at)pivotal(dot)io
>>>>>>>>> > wrote:
>>>>>>>>>
>>>>>>>>>> As discussed with Surinder, we have created a Redmine ticket for
>>>>>>>>>> his 4th comment regarding the error message not showing up when the app
>>>>>>>>>> can't be reached. This issue existed prior to this patch and should be
>>>>>>>>>> prioritized.
>>>>>>>>>>
>>>>>>>>>> https://redmine.postgresql.org/issues/2640
>>>>>>>>>>
>>>>>>>>>> Thanks!
>>>>>>>>>> Matt & Sarah
>>>>>>>>>>
>>>>>>>>>> On Wed, Aug 9, 2017 at 4:06 PM, Sarah McAlear <
>>>>>>>>>> smcalear(at)pivotal(dot)io> wrote:
>>>>>>>>>>
>>>>>>>>>>> Hi Surinder!
>>>>>>>>>>>
>>>>>>>>>>> I am not able to see anything different from what I see on
>>>>>>>>>>> Master with or without the patch applied. I tried adjusting the
>>>>>>>>>>> preferences. I did update the dashboard.js to instantiate a new object,
>>>>>>>>>>> great idea!
>>>>>>>>>>>
>>>>>>>>>>> Thanks,
>>>>>>>>>>> Sarah
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> On Wed, Aug 9, 2017 at 1:42 PM, Surinder Kumar <
>>>>>>>>>>> surinder(dot)kumar(at)enterprisedb(dot)com> wrote:
>>>>>>>>>>>
>>>>>>>>>>>> Hi Wenlin,
>>>>>>>>>>>>
>>>>>>>>>>>> On Tue, Aug 8, 2017 at 3:15 PM, Wenlin Zhang <wzhang(at)pivotal(dot)io
>>>>>>>>>>>> > wrote:
>>>>>>>>>>>>
>>>>>>>>>>>>> Hi Surinder,
>>>>>>>>>>>>>
>>>>>>>>>>>>> Thanks for your review.
>>>>>>>>>>>>>
>>>>>>>>>>>>> We have changed the indentation for _dashboard.scss file
>>>>>>>>>>>>> and also removed the style about icon-postgres:before, like
>>>>>>>>>>>>> margin-top,etc, but we are not sure if it is perfectly aligned now, you can
>>>>>>>>>>>>> add further change to it.
>>>>>>>>>>>>>
>>>>>>>>>>>>> As the second comment, I'm sorry I'm not sure what's the
>>>>>>>>>>>>> problem, could you please clarify it? Because we replace css with scss
>>>>>>>>>>>>> right now, dashboard.css doesn't exist. So maybe you are looking at the css
>>>>>>>>>>>>> file that are complied by the scss?
>>>>>>>>>>>>>
>>>>>>>>>>>> ​Sorry I​ typed 'dashboard.css' instead of 'dashboard.js'.
>>>>>>>>>>>> ​In dashboard.js can we change `return DashboardAlert;` to
>>>>>>>>>>>> `return new DashboardAlert();`
>>>>>>>>>>>> and then we can remove the instances being created(var
>>>>>>>>>>>> alertDashboard = new AlertDashboard();) from dashboard.js, and simply
>>>>>>>>>>>> use `AlertDashboard.info('message')`.
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>> For the fourth comment, we tried the steps you suggested
>>>>>>>>>>>>> on master branch, the error is not shown either. So it should be an
>>>>>>>>>>>>> existing issue. But if you want to see the error message, navigate to
>>>>>>>>>>>>> "Servers" at the top of browser, then navigate back to postgresql server,
>>>>>>>>>>>>> then you will see the error message.
>>>>>>>>>>>>>
>>>>>>>>>>>> ​The error in console will appear when you selected a database
>>>>>>>>>>>> which is connected and stop the backend Python server because the request
>>>>>>>>>>>> for graphs for database level will fail and there is no response returned
>>>>>>>>>>>> from server.
>>>>>>>>>>>> It might be not reproducible to you If you set the refresh rate
>>>>>>>>>>>> to higher value (i.e. Preferences > Graphs) in preferences. Just set
>>>>>>>>>>>> refresh rate to 1 for all dashboard graphs and repeat the steps.
>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>
>>>>>>>>>>>>> Wenlin &Violet
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> On Mon, Aug 7, 2017 at 2:30 PM, Surinder Kumar <
>>>>>>>>>>>>> surinder(dot)kumar(at)enterprisedb(dot)com> wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>>> Hi
>>>>>>>>>>>>>> Review comments:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> 1.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> For consistency, we use two spaces for indentation in CSS
>>>>>>>>>>>>>> files. Four spaces are used in _dashboard.scss file. The
>>>>>>>>>>>>>> configurations are defined in web/.editorconfig file.
>>>>>>>>>>>>>> 2.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> In,dashboard.css Can we return function object in return
>>>>>>>>>>>>>> instead of function class itself, this will eliminate the need of creating
>>>>>>>>>>>>>> function object every time we use info and error?
>>>>>>>>>>>>>> 3.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> On Dashboard, I can see Postgres icon is misaligned
>>>>>>>>>>>>>> compared to other icons in Getting Started section. It is
>>>>>>>>>>>>>> not related to this patch. adjusting margin top will fix it.
>>>>>>>>>>>>>> 4.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> I tried to test out Error message displayed, but I
>>>>>>>>>>>>>> encounter an error(screenshot attached).
>>>>>>>>>>>>>> Steps to reproduce:
>>>>>>>>>>>>>> - Open pgAdmin4 in browser
>>>>>>>>>>>>>> - Connect to PostgreSQL Server, Keep dashboard tab
>>>>>>>>>>>>>> open.
>>>>>>>>>>>>>> - Navigate to the database which is connected.
>>>>>>>>>>>>>> - Now disconnect pgAdmin4 python server.
>>>>>>>>>>>>>> ​
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> ​Here I mean Stop Python server. I​
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>> 1.
>>>>>>>>>>>>>> - ​
>>>>>>>>>>>>>> - No error message is displayed on Dashboard because
>>>>>>>>>>>>>> it breaks in JS as xhr.responseText is empty.
>>>>>>>>>>>>>> However, it might be an existing issue.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>> Surinder
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> On Mon, Aug 7, 2017 at 10:40 AM, Wenlin Zhang <
>>>>>>>>>>>>>> wzhang(at)pivotal(dot)io> wrote:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Hi Ashesh,
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> That's correct. This patch just changed alert style in
>>>>>>>>>>>>>>> the 'tabs', such as Dependency and Dependents.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Thanks
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Wenlin
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> On Mon, Aug 7, 2017 at 12:51 PM, Ashesh Vashi <
>>>>>>>>>>>>>>> ashesh(dot)vashi(at)enterprisedb(dot)com> wrote:
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Surinder,
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Please take a look at this patch.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> If I recalls correctly, this patch is related to styling of
>>>>>>>>>>>>>>>> the 'tabs' shown on the main window.
>>>>>>>>>>>>>>>> Wenlin - please correct me if my understanding is wrong.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> --
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Thanks & Regards,
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Ashesh Vashi
>>>>>>>>>>>>>>>> EnterpriseDB INDIA: Enterprise PostgreSQL Company
>>>>>>>>>>>>>>>> <http://www.enterprisedb.com>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> *http://www.linkedin.com/in/asheshvashi*
>>>>>>>>>>>>>>>> <http://www.linkedin.com/in/asheshvashi>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> On Mon, Aug 7, 2017 at 8:11 AM, Sarah McAlear <
>>>>>>>>>>>>>>>> smcalear(at)pivotal(dot)io> wrote:
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Hi hackers,
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Could you please review this patch?
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Thanks
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Wenlin and Sarah
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> On Wed, Aug 2, 2017 at 2:15 PM, Wenlin Zhang <
>>>>>>>>>>>>>>>>> wzhang(at)pivotal(dot)io> wrote:
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> Hi Hackers,
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> This patch changes the alert style in the sub-navigation
>>>>>>>>>>>>>>>>>> to match style guide.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>>>>> Wenlin, Shirley & Sarah
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> ​
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>

--
*Akshay Joshi*
*Principal Software Engineer *

*Phone: +91 20-3058-9517Mobile: +91 976-788-8246*