Re: [pgAdmin4][Patch]: Allow user to choose background colour for server

Thanks! Patch applied, with a minor change to default the query tool header
to a white background if a custom foreground colour is specified (otherwise
you'd get blue).

On Tue, Nov 21, 2017 at 12:07 PM, Murtuza Zabuawala <
murtuza(dot)zabuawala(at)enterprisedb(dot)com> wrote:

> 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(at)pgadmin(dot)org> wrote:
>>
>>> Hi
>>>
>>> On Mon, Nov 20, 2017 at 5:47 PM, Murtuza Zabuawala <
>>> murtuza(dot)zabuawala(at)enterprisedb(dot)com> wrote:
>>>
>>>> On Mon, Nov 20, 2017 at 10:40 PM, Dave Page <dpage(at)pgadmin(dot)org> wrote:
>>>>
>>>>> Hi
>>>>>
>>>>> On Mon, Nov 20, 2017 at 4:19 PM, Murtuza Zabuawala <
>>>>> murtuza(dot)zabuawala(at)enterprisedb(dot)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
>>>> <http://www.spitalpharmazie-basel.ch/aml/assets/js/aci-tree/documentation.html#.addIcon>
>>>> 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(at)pgadmin(dot)org> wrote:
>>>>>>
>>>>>>> Hi
>>>>>>>
>>>>>>> On Fri, Nov 17, 2017 at 9:30 AM, Murtuza Zabuawala <
>>>>>>> murtuza(dot)zabuawala(at)enterprisedb(dot)com> wrote:
>>>>>>>
>>>>>>>> Hi Dave,
>>>>>>>>
>>>>>>>> PFA updated patch.
>>>>>>>>
>>>>>>>> On Thu, Nov 16, 2017 at 6:34 PM, Dave Page <dpage(at)pgadmin(dot)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
>>>
>>
>>
>

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

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