On Tue, Nov 21, 2017 at 5:37 PM, Murtuza Zabuawala < murtuza.zabuaw...@enterprisedb.com> wrote:
> Hi Dave, > > PFA updated patch with changes suggested by Ashesh. > Hopefully the last revision :) > > +1 :) > > 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 <dp...@pgadmin.org> wrote: >> >>> Hi >>> >>> On Mon, Nov 20, 2017 at 5:47 PM, Murtuza Zabuawala < >>> murtuza.zabuaw...@enterprisedb.com> wrote: >>> >>>> On Mon, Nov 20, 2017 at 10:40 PM, Dave Page <dp...@pgadmin.org> wrote: >>>> >>>>> Hi >>>>> >>>>> On Mon, Nov 20, 2017 at 4:19 PM, Murtuza Zabuawala < >>>>> murtuza.zabuaw...@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 >>>> <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 <dp...@pgadmin.org> wrote: >>>>>> >>>>>>> Hi >>>>>>> >>>>>>> On Fri, Nov 17, 2017 at 9:30 AM, Murtuza Zabuawala < >>>>>>> murtuza.zabuaw...@enterprisedb.com> wrote: >>>>>>> >>>>>>>> Hi Dave, >>>>>>>> >>>>>>>> PFA updated patch. >>>>>>>> >>>>>>>> On Thu, Nov 16, 2017 at 6:34 PM, Dave Page <dp...@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 >>> >> >> >
