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 <vch...@pivotal.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 <smcal...@pivotal.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 <vch...@pivotal.io> wrote: >> >>> Here's the Redmine link >>> >>> https://redmine.postgresql.org/issues/2644 >>> >>> On Thu, Aug 10, 2017 at 4:03 PM, Violet Cheng <vch...@pivotal.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.ku...@enterprisedb.com> wrote: >>>> >>>>> Please find attached screenshot. >>>>> >>>>> On Thu, Aug 10, 2017 at 11:30 AM, Surinder Kumar < >>>>> surinder.ku...@enterprisedb.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.ku...@enterprisedb.com> wrote: >>>>>> >>>>>>> Hi, >>>>>>> >>>>>>> The updated patch looks good to me. >>>>>>> >>>>>>> On Wed, Aug 9, 2017 at 4:15 PM, Sarah McAlear <smcal...@pivotal.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 <smcal...@pivotal.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.ku...@enterprisedb.com> wrote: >>>>>>>>> >>>>>>>>>> Hi Wenlin, >>>>>>>>>> >>>>>>>>>> On Tue, Aug 8, 2017 at 3:15 PM, Wenlin Zhang <wzh...@pivotal.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.ku...@enterprisedb.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 < >>>>>>>>>>>> wzh...@pivotal.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.va...@enterprisedb.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 < >>>>>>>>>>>>>> smcal...@pivotal.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 < >>>>>>>>>>>>>>> wzh...@pivotal.io> wrote: >>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> Hi Hackers, >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> This patch changes the alert style in the sub-navigation to >>>>>>>>>>>>>>>> match style guide. >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> Thanks, >>>>>>>>>>>>>>>> Wenlin, Shirley & Sarah >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>> >>>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>>> >>>>>> >>>>> >>>> >>> >> >
