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

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