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

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