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

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.

- 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
>