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

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