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

Hi

On Tue, Nov 12, 2019 at 4:59 AM Aditya Toshniwal <
[email protected]> wrote:

> Hi Hackers,
>
> Attached is a patch to address the review comments.
>
> On Mon, Nov 11, 2019 at 9:13 PM Dave Page <[email protected]> wrote:
>
>> Hi
>>
>> On Mon, Nov 11, 2019 at 3:11 PM Aditya Toshniwal <
>> [email protected]> wrote:
>>
>>> Hi Dave,
>>>
>>> On Mon, Nov 11, 2019, 20:33 Dave Page <[email protected]> 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. :/
>

So I eventually found that I had a rogue Python process running pgAdmin
that somehow got detached from PyCharm. Killed that, restarted, and now it
looks good.


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

Looks good.


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

Attached. It may just be that it looks a little sharp because it's a long
line. Maybe we should just make it a darker shade entirely.

[image: Screenshot 2019-11-12 at 09.09.42.png]

BTW, what was the reason for removing the styling from the scroll bars?


>>
>>>
>>>> Thanks!
>>>>
>>>> On Mon, Nov 11, 2019 at 1:22 PM Akshay Joshi <
>>>> [email protected]> 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 <
>>>>> [email protected]> wrote:
>>>>>
>>>>>> Hi Hackers,
>>>>>>
>>>>>> Attached is the updated patch.
>>>>>> Kindly review.
>>>>>>
>>>>>> On Mon, Nov 11, 2019 at 3:42 PM Aditya Toshniwal <
>>>>>> [email protected]> wrote:
>>>>>>
>>>>>>> Hi,
>>>>>>>
>>>>>>> Kindly hold on with the patch. Few more changes required per review
>>>>>>> by @Ashesh Vashi <[email protected]> .
>>>>>>>
>>>>>>> On Mon, Nov 11, 2019 at 3:07 PM Aditya Toshniwal <
>>>>>>> [email protected]> 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 <[email protected]> for inputs on that.
>>>>>>>>
>>>>>>>> Kindly review.
>>>>>>>>
>>>>>>>> On Mon, Nov 11, 2019 at 3:00 PM Aditya Toshniwal <
>>>>>>>> [email protected]> wrote:
>>>>>>>>
>>>>>>>>> Hi Dave,
>>>>>>>>>
>>>>>>>>> On Mon, Nov 11, 2019 at 2:38 PM Dave Page <[email protected]>
>>>>>>>>> wrote:
>>>>>>>>>
>>>>>>>>>> Hi
>>>>>>>>>>
>>>>>>>>>> On Mon, Nov 11, 2019 at 7:01 AM Aditya Toshniwal <
>>>>>>>>>> [email protected]> wrote:
>>>>>>>>>>
>>>>>>>>>>> Hi Dave,
>>>>>>>>>>>
>>>>>>>>>>> On Thu, Nov 7, 2019 at 7:56 PM Dave Page <[email protected]>
>>>>>>>>>>> wrote:
>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> On Thu, Nov 7, 2019 at 2:18 PM Dave Page <[email protected]>
>>>>>>>>>>>> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> On Thu, Nov 7, 2019 at 1:25 PM Akshay Joshi <
>>>>>>>>>>>>> [email protected]> wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>>> Thanks, patch applied.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> On Thu, Nov 7, 2019 at 6:39 PM Aditya Toshniwal <
>>>>>>>>>>>>>> [email protected]> 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"
>


-- 
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company