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

Awesome, thanks!

Akshay Joshi <akshay(dot)joshi(at)enterprisedb(dot)com>于2017年8月17日 周四下午8:24写道：

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