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

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