Re: Pgadmin4 System Stats Extension Design

Hi Sahil,

Your PR is showing 135 file changes and a lot of commits which shouldn't have appeared on your PR.

It is very difficult to identify your changes. Can you please check once?

On Fri, Sep 8, 2023 at 1:13 AM Sahil Harpal <sahilharpal1234@gmail.com> wrote:

Hi Aditya,

Sorry for the delay; I've been a bit busy lately. I have made all the requested changes. Could you please review it?

Thanks,

Sahil

On Mon, 4 Sept 2023 at 11:17, Aditya Toshniwal <aditya.toshniwal@enterprisedb.com> wrote:

Hi Sahil,

I have replied to the PR.

On Sun, Sep 3, 2023 at 2:31 AM Sahil Harpal <sahilharpal1234@gmail.com> wrote:

Hi Aditya,

I have made almost all of the requested changes and pushed the latest code. I just need a bit of clarification for a couple of suggestions that I have posted in the reviews.

Thank you,

Sahil

On Thu, 31 Aug 2023 at 17:20, Aditya Toshniwal <aditya.toshniwal@enterprisedb.com> wrote:

Hi Sahil,

OK fine. We will check it later. Not priority. Please also fix the review raised on PR.

On Thu, Aug 31, 2023, 16:17 Sahil Harpal <sahilharpal1234@gmail.com> wrote:

Hi Aditya,

I have made all these changes except the StreamingChart issue. I tried filling an array with null values inside the StreamingChart component while initializing initialState but still the issue is not resolved.

I have made following changes:

 const initialState = [
    Array.from(new Array(xRange).keys()),
    ...(data.datasets?.map((d)=>{
      let nullValues = new Array(xRange - d.data.length).fill(null);
      let ret = [...nullValues, ...d.data];
      ret.reverse();
      return ret;
    })??{}),
  ];

It works fine if we initialize the data array with the null values but I'm not getting why this is not working. 

Thank you,

Sahil

Sender notified by Mailtrack

31/08/23, 16:14:27

On Mon, 28 Aug 2023 at 10:44, Aditya Toshniwal <aditya.toshniwal@enterprisedb.com> wrote:

Hi Sahil,

I have few observations. You have added separate titles for graphs and other tabular data. This is inconsistent with existing UI.

For example,

like here:

like here:

The dashboard goes blank when I change refresh rates.

And regarding filling with nulls to fix the reversing issue of graph - you can do it in StreamingChart itself when setting initialState var, as it is StreamingChart's responsibility to do it.

Next review will be on the PR directly.

On Sun, Aug 27, 2023 at 5:58 PM Sahil Harpal <sahilharpal1234@gmail.com> wrote:

Hello everyone,

I have raised the PR.

I would like to request you all to review the changes and provide your valuable feedback. Your insights and suggestions would be invaluable in ensuring the quality and accuracy of the codebase.

Thank you,

Sahil

On Sun, 27 Aug 2023 at 07:13, Khushboo Vashi <khushboo.vashi@enterprisedb.com> wrote:

On Sat, 26 Aug 2023, 11:36 Sahil Harpal, <sahilharpal1234@gmail.com> wrote:

Hi Khushboo,

On Mon, 21 Aug 2023 at 10:03, Khushboo Vashi <khushboo.vashi@enterprisedb.com> wrote:

Sahil, once the issues get resolved, please raise the PR and we will do the final review there.

Could you please tell me to which branch I should raise the PR? 

Master branch

Also, should I remove the code responsible for the static DonutChart of process information?

Thank you,

Sahil

--

Thanks,

Aditya Toshniwal

pgAdmin Hacker | Sr. Software Architect | enterprisedb.com

"Don't Complain about Heat, Plant a TREE"

--

Thanks,

Aditya Toshniwal

pgAdmin Hacker | Sr. Software Architect | enterprisedb.com

"Don't Complain about Heat, Plant a TREE"

--

Thanks,

Aditya Toshniwal

pgAdmin Hacker | Sr. Software Architect | enterprisedb.com

"Don't Complain about Heat, Plant a TREE"