Re: [pgAdmin4][Patch]: Allow user to choose background colour for server - Mailing list pgadmin-hackers

Hi Dave,

PFA updated patch with changes suggested by Ashesh.

Hopefully the last revision :)

On Tue, Nov 21, 2017 at 4:12 PM, Murtuza Zabuawala <murtuza.zabuawala@enterprisedb.com> wrote:

Hi Dave,

Please find updated patch.

On Tue, Nov 21, 2017 at 2:43 PM, Dave Page <dpage@pgadmin.org> wrote:

Hi

On Mon, Nov 20, 2017 at 5:47 PM, Murtuza Zabuawala <murtuza.zabuawala@enterprisedb.com> wrote:

On Mon, Nov 20, 2017 at 10:40 PM, Dave Page <dpage@pgadmin.org> wrote:

Hi

On Mon, Nov 20, 2017 at 4:19 PM, Murtuza Zabuawala <murtuza.zabuawala@enterprisedb.com> wrote:

Hi Dave,

PFA updated patch.

OK, that's looking better. I just found a couple of smaller issue this time:

- Why can't I select a foreground colour unless I select a non-white background? That seems inconvenient and unnecessary

​Because we can only pass CSS classes to aciTree node which it stores in html element and we are passing bg & fg colors as additional classes like <icon_class> <bg_color> <fg_color>, 

When we fetch the class list from html element, we get array of classes, so if we have any value at index 1 then it is bgcolor and we have value at index 2 then it is fgcolor, and using these css colors then we create dynamic style tag for the acitree node which inject at runtime in DOM.

 

So we can only identify them by index, so we have disabled the fgcolor field if bgcolor field is not provided.

Implementation details like this are really not a good reason to confuse the user. Why can't we just default the background colour to white if unspecified (which it will be anyway), thus ensuring there is always the expected number of elements? Then, the user can specify (for example) red on white - which I suspect would be a popular choice (or variations thereof).

 

- If I set a foreground and background colour, and then later set the background to a new colour and the foreground to "no colour", the background change takes effect immediately, but the foreground change requires a refresh (changing to a different foreground colour seems to work fine though). 

​Fixed, Older style tag was not cleaning up properly and it was pickking up color from previous style tag.​

 

 

On Mon, Nov 20, 2017 at 6:48 PM, Dave Page <dpage@pgadmin.org> wrote:

Hi

On Fri, Nov 17, 2017 at 9:30 AM, Murtuza Zabuawala <murtuza.zabuawala@enterprisedb.com> wrote:

Hi Dave,

PFA updated patch.

On Thu, Nov 16, 2017 at 6:34 PM, Dave Page <dpage@pgadmin.org> wrote:

Hi

Looks good. A few changes/suggestions:

- There seem to be some debugger calls left in the code, e.g. in editors.js

​Fixed

- Instead of bgcolor and font_color, lets use bgcolor and fgcolor (foreground). 

​Fixed​

 

- The docs are (technically) en_US, so we should use color not colour in them.

​Fixed​

 

- If the colours have been set for a server, I think we should also colour the title bar in the query tool. The only possible problem there is that the current default colours are reversed in comparison to the treeview (e.g. the treeview defaults to black text, whilst the query tool title bar is white on blue. Not sure if that is really an issue or not.

What do you think?

​I have added logic to set the background & foreground colour in query tool & datagrid title bar.

- Default title bar colours, b

ackground

​: blue 

foreground

​: white 

 [

​w​

hat we have right now]

- When user has custom background colour for the server​, 

b

ackground

​: <

custom background colour

> 

foreground

​: black

- When user has custom background colour as well as foreground colour for the server​, 

b

ackground

​: <

custom background colour

> 

foreground

​: 

<

custom 

​foreground

 colour

>

I think this looks good now for the most part, except:

- The default colours for the title bar on the query tool seem to be black on white. See the first screenshot.

​Fixed​

 

- If I just set the foreground colour for a connection, it only shows up in the query tool title, not on the treeview (and the query tool title has a white background (which does seem reasonable, as the treeview would as well). See the second screenshot.

​Fixed.​

 

--

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

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--

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

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--

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

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company