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

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