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

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(dot)zabuawala(at)enterprisedb(dot)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
>>
>
>