Re: [pgAdmin][RM4348] Theme options in pgAdmin and dark theme

Hi Hackers,

Attached is a patch to address the review comments.

On Mon, Nov 11, 2019 at 9:13 PM Dave Page <dpage(at)pgadmin(dot)org> wrote:

> Hi
>
> On Mon, Nov 11, 2019 at 3:11 PM Aditya Toshniwal <
> aditya(dot)toshniwal(at)enterprisedb(dot)com> wrote:
>
>> Hi Dave,
>>
>> On Mon, Nov 11, 2019, 20:33 Dave Page <dpage(at)pgadmin(dot)org> wrote:
>>
>>> Hi
>>>
>>> That's looking a lot better. I see a couple of issues (screenshots
>>> attached):
>>>
>>> - The combo box on the query tool toolbar isn't dark.
>>>
>> I'm not sure why you are not able to see it but I've changed it. Try a
>> python restart.
>>
>
> I've done a make clean/make bundle, restarted Python, and done a hard
> reload in the browser to no joy. FYI, I'm running in Chrome 78.0.3904.70 on
> macOS.
>
[image: Screenshot 2019-11-12 at 10.03.59.png]
I'm not seeing it. Pulled latest, bundled and restarted. :/

>
>> - The tagline on the dashboard logo isn't readable.
>>>
>> Yes because that's an SVG image. We may need to make it completely from
>> HTML.
>>
>
> Can we have an alternate image? Maybe if it's served through a flask route
> and we look for /logo.png?t=dark or something like that to select the image
> version?
>
Or better embed the SVG so that we can change colors using CSS. Patch
attached. You can tweak $color-brand to change the welcome dashboards blue
color.

>
>
>>
>>> A couple of other thoughts:
>>>
>>> - The guide lines on the graphs are still too bright. Let's make them
>>> the same colour as the graph boarders please.
>>>
>> There is technical limitation here. To change those grid lines based on
>> theme we need to import variables in JS. I didn't find a way to dynamically
>> load SASS variables based on theme.
>> So I chose color which would work for both dark and black backgrounds and
>> is hard coded in JS.
>>
>
> Urgh, OK. We'll need to think about that then.
>
>
>>
>>> - The header bar foreground colour and the dividing white line below it
>>> seem quite harsh. Are they pure white? They should be softened a little bit.
>>>
>> I'll check the color, but AFAIR it's not pure white.
>>
>
> OK, thanks!
>
I didn't find anyone using white color. Could you please share a screenshot
? I've changed the foreground from #ddd to #d4d4d4 to soften a bit.

>
>
>>
>>> Thanks!
>>>
>>> On Mon, Nov 11, 2019 at 1:22 PM Akshay Joshi <
>>> akshay(dot)joshi(at)enterprisedb(dot)com> wrote:
>>>
>>>> Thanks, patch applied.
>>>>
>>>> As per discussion with Aditya, we have removed customized scroll bars
>>>> for the time being as they are not clearly visible with some of the
>>>> components.
>>>>
>>>> On Mon, Nov 11, 2019 at 5:25 PM Aditya Toshniwal <
>>>> aditya(dot)toshniwal(at)enterprisedb(dot)com> wrote:
>>>>
>>>>> Hi Hackers,
>>>>>
>>>>> Attached is the updated patch.
>>>>> Kindly review.
>>>>>
>>>>> On Mon, Nov 11, 2019 at 3:42 PM Aditya Toshniwal <
>>>>> aditya(dot)toshniwal(at)enterprisedb(dot)com> wrote:
>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> Kindly hold on with the patch. Few more changes required per review
>>>>>> by @Ashesh Vashi <ashesh(dot)vashi(at)enterprisedb(dot)com> .
>>>>>>
>>>>>> On Mon, Nov 11, 2019 at 3:07 PM Aditya Toshniwal <
>>>>>> aditya(dot)toshniwal(at)enterprisedb(dot)com> wrote:
>>>>>>
>>>>>>> Hi Hackers,
>>>>>>>
>>>>>>> Attached is the patch for further improvements in the Dark theme
>>>>>>> colors.
>>>>>>> Gray shades and other colors are changed to identify different
>>>>>>> components more clearly. Few of the controls were missing the privileges of
>>>>>>> dark theme, fixed that.
>>>>>>> Few dashboard graph related changes.
>>>>>>> As suggested, theme related code changes is removed from config.py
>>>>>>> and moved to miscellaneous under a new package - Themes. Thank you @Ashesh
>>>>>>> Vashi <ashesh(dot)vashi(at)enterprisedb(dot)com> for inputs on that.
>>>>>>>
>>>>>>> Kindly review.
>>>>>>>
>>>>>>> On Mon, Nov 11, 2019 at 3:00 PM Aditya Toshniwal <
>>>>>>> aditya(dot)toshniwal(at)enterprisedb(dot)com> wrote:
>>>>>>>
>>>>>>>> Hi Dave,
>>>>>>>>
>>>>>>>> On Mon, Nov 11, 2019 at 2:38 PM Dave Page <dpage(at)pgadmin(dot)org>
>>>>>>>> wrote:
>>>>>>>>
>>>>>>>>> Hi
>>>>>>>>>
>>>>>>>>> On Mon, Nov 11, 2019 at 7:01 AM Aditya Toshniwal <
>>>>>>>>> aditya(dot)toshniwal(at)enterprisedb(dot)com> wrote:
>>>>>>>>>
>>>>>>>>>> Hi Dave,
>>>>>>>>>>
>>>>>>>>>> On Thu, Nov 7, 2019 at 7:56 PM Dave Page <dpage(at)pgadmin(dot)org>
>>>>>>>>>> wrote:
>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> On Thu, Nov 7, 2019 at 2:18 PM Dave Page <dpage(at)pgadmin(dot)org>
>>>>>>>>>>> wrote:
>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> On Thu, Nov 7, 2019 at 1:25 PM Akshay Joshi <
>>>>>>>>>>>> akshay(dot)joshi(at)enterprisedb(dot)com> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>>> Thanks, patch applied.
>>>>>>>>>>>>>
>>>>>>>>>>>>> On Thu, Nov 7, 2019 at 6:39 PM Aditya Toshniwal <
>>>>>>>>>>>>> aditya(dot)toshniwal(at)enterprisedb(dot)com> wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>>> Hi Hackers,
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Attached is the updated patch with few more changes and
>>>>>>>>>>>>>> corrections.
>>>>>>>>>>>>>> Kindly review.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>> I've committed a couple of minor tweaks - one to remove a
>>>>>>>>>>>> space, e.g.
>>>>>>>>>>>>
>>>>>>>>>>>> gettext('A page refresh is required to apply the theme. Do you wish to refresh the page now ?'),
>>>>>>>>>>>>
>>>>>>>>>>>> is now:
>>>>>>>>>>>>
>>>>>>>>>>>> gettext('A page refresh is required to apply the theme. Do you wish to refresh the page now?'),
>>>>>>>>>>>>
>>>>>>>>>>>> And another change to fix the word wrapping in the README which
>>>>>>>>>>>> was totally different from the rest of the file.
>>>>>>>>>>>>
>>>>>>>>>>>> Thanks!
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Oh, and do we need all the code in config.py? I really didn't
>>>>>>>>>>> even want a config option in there to turn theming on or off (what's the
>>>>>>>>>>> point?), let alone 20 new lines.
>>>>>>>>>>>
>>>>>>>>>> The code is added after the config_local and config_distro is
>>>>>>>>>> loaded. So, user won't be able to disable it unless he directly changes the
>>>>>>>>>> config.py.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> That is clearly wrong and needs to be fixed. config_local and
>>>>>>>>> config_distro should be able to override anything in config.py.
>>>>>>>>>
>>>>>>>>> But... why allow the themes to be updated or disabled at all? It's
>>>>>>>>> not like a non-developer can add new ones, and it's not a security issue
>>>>>>>>> that an administrator might need to control. In fact, it's arguably an
>>>>>>>>> accessibility feature, for those whose eyes (like mine) last the day better
>>>>>>>>> with a darker theme.
>>>>>>>>>
>>>>>>>>> Let's remove it entirely please. I don't see any good reason to
>>>>>>>>> have any of that in config.py.
>>>>>>>>>
>>>>>>>> Intention is not to allow disabling the themes, but it's the
>>>>>>>> feature implementation code. I'll move out the code.
>>>>>>>>
>>>>>>>>>
>>>>>>>>> Thanks.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>> I'll reduce the code a bit.
>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> --
>>>>>>>>>>> Dave Page
>>>>>>>>>>> Blog: http://pgsnake.blogspot.com
>>>>>>>>>>> Twitter: @pgsnake
>>>>>>>>>>>
>>>>>>>>>>> EnterpriseDB UK: http://www.enterprisedb.com
>>>>>>>>>>> The Enterprise PostgreSQL Company
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> --
>>>>>>>>>> Thanks and Regards,
>>>>>>>>>> Aditya Toshniwal
>>>>>>>>>> Sr. Software Engineer | EnterpriseDB India | Pune
>>>>>>>>>> "Don't Complain about Heat, Plant a TREE"
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> --
>>>>>>>>> Dave Page
>>>>>>>>> Blog: http://pgsnake.blogspot.com
>>>>>>>>> Twitter: @pgsnake
>>>>>>>>>
>>>>>>>>> EnterpriseDB UK: http://www.enterprisedb.com
>>>>>>>>> The Enterprise PostgreSQL Company
>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> --
>>>>>>>> Thanks and Regards,
>>>>>>>> Aditya Toshniwal
>>>>>>>> Sr. Software Engineer | EnterpriseDB India | Pune
>>>>>>>> "Don't Complain about Heat, Plant a TREE"
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> --
>>>>>>> Thanks and Regards,
>>>>>>> Aditya Toshniwal
>>>>>>> Sr. Software Engineer | EnterpriseDB India | Pune
>>>>>>> "Don't Complain about Heat, Plant a TREE"
>>>>>>>
>>>>>>
>>>>>>
>>>>>> --
>>>>>> Thanks and Regards,
>>>>>> Aditya Toshniwal
>>>>>> Sr. Software Engineer | EnterpriseDB India | Pune
>>>>>> "Don't Complain about Heat, Plant a TREE"
>>>>>>
>>>>>
>>>>>
>>>>> --
>>>>> Thanks and Regards,
>>>>> Aditya Toshniwal
>>>>> Sr. Software Engineer | EnterpriseDB India | Pune
>>>>> "Don't Complain about Heat, Plant a TREE"
>>>>>
>>>>
>>>>
>>>> --
>>>> *Thanks & Regards*
>>>> *Akshay Joshi*
>>>>
>>>> *Sr. Software Architect*
>>>> *EnterpriseDB Software India Private Limited*
>>>> *Mobile: +91 976-788-8246*
>>>>
>>>
>>>
>>> --
>>> 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
>

--
Thanks and Regards,
Aditya Toshniwal
Sr. Software Engineer | EnterpriseDB India | Pune
"Don't Complain about Heat, Plant a TREE"