Received: from malur.postgresql.org ([2a02:16a8:dc51::56]) by arkaria.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA384:256) (Exim 4.89) (envelope-from ) id 1fb4bS-000161-7j for pgadmin-hackers@arkaria.postgresql.org; Thu, 05 Jul 2018 13:47:22 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.89) (envelope-from ) id 1fb4bO-00007a-87 for pgadmin-hackers@arkaria.postgresql.org; Thu, 05 Jul 2018 13:47:18 +0000 Received: from makus.postgresql.org ([2001:4800:1501:1::229]) by malur.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA384:256) (Exim 4.89) (envelope-from ) id 1fb4bN-00007T-M1 for pgadmin-hackers@lists.postgresql.org; Thu, 05 Jul 2018 13:47:18 +0000 Received: from mail-wm0-x241.google.com ([2a00:1450:400c:c09::241]) by makus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1fb4bI-00062X-7S for pgadmin-hackers@postgresql.org; Thu, 05 Jul 2018 13:47:16 +0000 Received: by mail-wm0-x241.google.com with SMTP id 69-v6so10875405wmf.3 for ; Thu, 05 Jul 2018 06:47:12 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pgadmin-org.20150623.gappssmtp.com; s=20150623; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=wgBNTAkD0frhORrPa3VtfVGvMzcT3lgOYvcb+boy44A=; b=D1vic3aHfGsTNhWCBJiJlYZvPoCrMfVCcytE+/RSvtZnNKvibQSzpVUPJ4sJhvHf1F UjP8vfcl0A2QQ7qSKLyo60uNcbwhXgVjUePxVLWY50LZiOT3xTmJ5qgpP/Fn7JzBzbDT 0VNkSuYFr0BbgiN4XtjywZf5asmAsusUQ/RCIQTkDJh+lAnQ3vtceGayFO4lPAGwGaD9 Ooxgs+MCCVPgBRnVzieXHekUf5RoO0jRjAoNddK/s19iOAaQa1xrb4yKZw+i0OkaSul0 kMxvy9t3qmYZzV8D6u6a5sjaHSpOpva8w1yF6PaattkwYAn2y7cdP2nn3We7v9wn67Pg K00g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=wgBNTAkD0frhORrPa3VtfVGvMzcT3lgOYvcb+boy44A=; b=LQN7pRRJuucHvhqsxIbJBQX5drdgpNKeCWIUPql9n1k9J2ULBY2FshNcPs+1AE9XQ0 F4KzTXvAAP+ayP228g1vOZM5t/0KLG3O/j2KuhG2rdZpF0Bid8ElRSa9HZ2j5UekqUr/ +9WD6D0MWne9jlb96SFgakHpIeXg/Q2Fr/SjVjaLMPqI8zu7ComXLEXITCQPvN6871fQ n9GHo/UTJp6Y+2aFwtpv4iXv0crKYu9kF6WbCYK2kYjv1N5cRJIpTqmVHwHhfBwRdMnp cawzgWJz2j/+zMVMhOSHqgyWqEStgK8yVyX7wqkBDrz+3+i4FE2qD6BFolUW0XsmHsrf CerA== X-Gm-Message-State: APt69E059E8Ebvk/vf1VB6e9ncLjgeXZFVJ7HJIgwsdQRcmTYbZg93nk F9dVPdXs3MPI17Tq39EybIWLXZVw2MSvbzI62IGj1A== X-Google-Smtp-Source: AAOMgpd3vszHruRV6NdUfuzBUMZY0U5iBKdH0Wh/g+4X8oyR7S/3+JHUKVTVG0ooFcNgOEILL1hOU7L5z7D7emeP20E= X-Received: by 2002:adf:adc9:: with SMTP id w67-v6mr4450217wrc.135.1530798429658; Thu, 05 Jul 2018 06:47:09 -0700 (PDT) MIME-Version: 1.0 Received: by 2002:a1c:8955:0:0:0:0:0 with HTTP; Thu, 5 Jul 2018 06:47:08 -0700 (PDT) In-Reply-To: References: From: Dave Page Date: Thu, 5 Jul 2018 14:47:08 +0100 Message-ID: Subject: Re: [pgAdmin4][Patch]: RM #3397 Add support for JIT stats in EXPLAIN output in PG11 To: Akshay Joshi Cc: pgadmin-hackers Content-Type: multipart/alternative; boundary="0000000000009f79df057040c862" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk --0000000000009f79df057040c862 Content-Type: text/plain; charset="UTF-8" Hi Looks good to me - the only remaining issue is that the button still isn't the same colours as the zoom ones. I haven't found all the differences, but there seems to be an 'opacity: 0.5" on the stats area, and the disabled attribute is still being set. On Thu, Jul 5, 2018 at 11:21 AM, Akshay Joshi wrote: > Hi Dave > > As per discussion attached is the modified patch with following review > comments has been fixed : > > - Hide statistics button if not applicable(no statistics to show). > - Extract 'StatisticsModel' into a separate file and added jasmine > test for this. > > > > > On Tue, Jul 3, 2018 at 6:40 PM, Akshay Joshi < > akshay.joshi@enterprisedb.com> wrote: > >> Hi Dave, >> >> Please ignore the previous patch, I have made some more changes related >> to background colour on enabled/disabled state. >> Attached is the modified patch. Please review it. >> >> On Tue, Jul 3, 2018 at 12:35 PM, Akshay Joshi < >> akshay.joshi@enterprisedb.com> wrote: >> >>> Hi Dave, >>> >>> On Mon, Jul 2, 2018 at 4:10 PM, Dave Page wrote: >>> >>>> Hi >>>> >>>> On Sat, Jun 30, 2018 at 9:15 AM, Akshay Joshi < >>>> akshay.joshi@enterprisedb.com> wrote: >>>> >>>>> Hi Dave >>>>> >>>>> On Fri, Jun 29, 2018 at 7:45 PM, Dave Page wrote: >>>>> >>>>>> >>>>>> >>>>>> On Fri, Jun 29, 2018 at 3:12 PM, Akshay Joshi < >>>>>> akshay.joshi@enterprisedb.com> wrote: >>>>>> >>>>>>> Hi Dave >>>>>>> >>>>>>> On Fri, Jun 29, 2018 at 6:56 PM, Dave Page >>>>>>> wrote: >>>>>>> >>>>>>>> Hi >>>>>>>> >>>>>>>> On Fri, Jun 29, 2018 at 9:55 AM, Akshay Joshi < >>>>>>>> akshay.joshi@enterprisedb.com> wrote: >>>>>>>> >>>>>>>>> Hi Hackers, >>>>>>>>> >>>>>>>>> Attached is the patch to fix the RM #3397 Add support for JIT >>>>>>>>> stats in EXPLAIN output in PG11. Please review it. >>>>>>>>> >>>>>>>> >>>>>>>> A couple of immediate thoughts: >>>>>>>> >>>>>>>> - When the canvas is first rendered, there's a vertical scrollbar >>>>>>>> now. As soon as I mouseover the new icon, it vanishes and the icon jumps to >>>>>>>> the right. >>>>>>>> >>>>>>> >>>>>>> Will look into it. Vertical scrollbar comes even if you remove >>>>>>> my patch and try to hover any image. >>>>>>> >>>>>>>> >>>>>>>> - The icon seems lighter than the other controls on the left. >>>>>>>> >>>>>>> >>>>>>> Same css has been applied, only difference is button is >>>>>>> disabled. >>>>>>> >>>>>>>> >>>>>>>> - The icon isn't disabled when there is no info to show. >>>>>>>> >>>>>>> >>>>>>> Button is always disabled, I have just change the opacity. >>>>>>> >>>>>>>> >>>>>>>> Thanks. >>>>>>>> >>>>>>> >>>>>> Maybe - but I can still click it and it reacts as if it's active. It >>>>>> may be lighter to indicate that it's disabled, but its not behaving as >>>>>> such. >>>>>> >>>>> >>>>> Attached is the modified patch. Please review it. >>>>> >>>> >>>> The button still changes foreground colour on mouseover when disabled. >>>> I think it needs to be completely non-reactive when disabled. It should >>>> also be a noticably lighter shade when disabled; right now it seems to be >>>> darker than the other buttons (see attached). >>>> >>> >>> Attached is the modified patch. Please review it. >>> >>>> >>>> -- >>>> Dave Page >>>> Blog: http://pgsnake.blogspot.com >>>> Twitter: @pgsnake >>>> >>>> EnterpriseDB UK: http://www.enterprisedb.com >>>> The Enterprise PostgreSQL Company >>>> >>> >>> >>> >>> -- >>> *Akshay Joshi* >>> >>> *Sr. Software Architect * >>> >>> >>> >>> *Phone: +91 20-3058-9517Mobile: +91 976-788-8246* >>> >> >> >> >> -- >> *Akshay Joshi* >> >> *Sr. Software Architect * >> >> >> >> *Phone: +91 20-3058-9517Mobile: +91 976-788-8246* >> > > > > -- > *Akshay Joshi* > > *Sr. Software Architect * > > > > *Phone: +91 20-3058-9517Mobile: +91 976-788-8246* > -- Dave Page Blog: http://pgsnake.blogspot.com Twitter: @pgsnake EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company --0000000000009f79df057040c862 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hi

Looks good to me - the only remainin= g issue is that the button still isn't the same colours as the zoom one= s. I haven't found all the differences, but there seems to be an 'o= pacity: 0.5" on the stats area, and the disabled attribute is still be= ing set.=C2=A0

On Thu, Jul 5, 2018 at 11:21 AM, Akshay Joshi &l= t;akshay= .joshi@enterprisedb.com> wrote:
Hi Dave

As per discussion attached= is the modified patch with following review comments has been fixed :
  • Hide statistics button if not applicable(no statistics to sho= w).
  • Extract 'StatisticsModel' into a separate file and adde= d jasmine test for this.=C2=A0=C2=A0


On Tue, Jul 3, 2018 at 6:40 PM, Akshay = Joshi <akshay.joshi@enterprisedb.com> wrote= :
Hi Dave,

Please ignore the previous patch, I have made some more changes relat= ed to background colour on enabled/disabled state.=C2=A0
At= tached is the modified patch. Please review it.=C2=A0 =C2=A0

On T= ue, Jul 3, 2018 at 12:35 PM, Akshay Joshi <akshay.joshi@enterp= risedb.com> wrote:
Hi Dave,

On = Mon, Jul 2, 2018 at 4:10 PM, Dave Page <dpage@pgadmin.org> w= rote:
Hi

On Sat, Jun 30, 2018 at 9:1= 5 AM, Akshay Joshi <akshay.joshi@enterprisedb.com>= wrote:
Hi Dave
On Fri, Jun 2= 9, 2018 at 7:45 PM, Dave Page <dpage@pgadmin.org> wrote:
=


On Fri, Jun 29, 2018 at 3:12 PM, A= kshay Joshi <akshay.joshi@enterprisedb.com>= wrote:
Hi Dave

On Fri, Jun 29, 2018= at 6:56 PM, Dave Page <dpage@pgadmin.org> wrote:
Hi

<= div class=3D"gmail_quote">On Fri, Jun 29, 2018 at 9:55 AM, Akshay Jos= hi <akshay.joshi@enterprisedb.com> wrote:
Hi Hackers,

Attached is the patch to fix the RM #3397=C2=A0Add support for JIT stats = in EXPLAIN output in PG11. Please review it.
<= br>
A couple of immediate thoughts:

- When the canvas is first rendered, there's a vertical scrollbar no= w. As soon as I mouseover the new icon, it vanishes and the icon jumps to t= he right.

= =C2=A0 =C2=A0 =C2=A0Will look into it. Vertical scrollbar comes even if you= remove my patch and try to hover any image.=C2=A0

- The icon seems lighter than the other c= ontrols on the left.

=C2=A0 =C2=A0 =C2=A0Same css has been applied, only difference is = button is disabled.=C2=A0

- The icon isn't disabled when there is no info to show.
=

=C2=A0 =C2=A0 =C2= =A0Button is always disabled, I have just change the opacity.=C2=A0 =C2=A0= =C2=A0

Thanks.=C2= =A0

Maybe - but I can still click it and it react= s as if it's active. It may be lighter to indicate that it's disabl= ed, but its not behaving as such.=C2=A0

=C2=A0 =C2=A0 Attached is the modified patch. P= lease review it.=C2=A0=C2=A0

<= /div>
The button still changes foreground colour on mouseover wh= en disabled.=C2=A0 I think it needs to be completely non-reactive when disa= bled. It should also be a noticably lighter shade when disabled; right now = it seems to be darker than the other buttons (see attached).

=C2=A0 =C2=A0Attached is the modi= fied patch. Please review it.=C2=A0

--
Dave= Page
Blog: ht= tp://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: = http://www.enterp= risedb.com
The Enterprise PostgreSQL Company



-= -
Akshay Joshi
= Sr. Software Architect
=


Phone: +91 20-3058-9517<= br>Mobile: +91 976-788-8246


--
Akshay Joshi
Sr. Sof= tware Architect

Phone: +91 20-3058-9517
Mobile: +91 976-788-8246<= /b>



--
=
Akshay Joshi
Sr. Software Archit= ect

=


--
=
Dave Page=
Blog: http://= pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprised= b.com
The Enterprise PostgreSQL Company
--0000000000009f79df057040c862--