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

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.zabuawala@
> enterprisedb.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
>