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

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