Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change HtmlDumper to use light theme #12857

Merged
merged 1 commit into from
Mar 11, 2023

Conversation

engram-design
Copy link
Contributor

This might be a contentious change, but in Craft 4.4 logging was changed to use Symfony VarDumper which is a neat choice. I'm not a massive fan of things being collapsed by default, but control+click expands all.

See #12479

Personally, I find the dark colours very jarring on a blank white screen. Even if the OS is set to use dark mode, the browser screen will always be white, because this is a "dump-and-die" call. Changing this renders things a little lighter:

image

Side note: I'm also happy to develop a "craftcms" theme to reflect how it was (larger font, colours), but it requires creating a new HtmlDumper class to extend things, and I honestly wasn't sure where that should sit in the codebase.

If you're down for that, let me know where to put it and can alter that. Or, you might be happy with how it is.

@engram-design engram-design requested a review from a team as a code owner March 9, 2023 22:48
@bencroker
Copy link
Contributor

Thanks @engram-design, massive improvement in just 1 line of code!

@mmikkel
Copy link
Contributor

mmikkel commented Mar 9, 2023

🔥

@wsydney76
Copy link

wsydney76 commented Mar 10, 2023

Support for this!

Made my own little dumper functions a while ago, because I can't read dark mode in general and the small font size in particular.

Used this config, which is just the default light theme with a bigger font size and some (Tailwind...) colors for better contrast, that pass an 'AA' check in Chrome's CSS overview:

'light' => [
    'default' => 'background:none; color:#CC7832; line-height:1.2em; font:16px Menlo, Monaco, Consolas, monospace; word-wrap: break-word; white-space: pre-wrap; position:relative; z-index:99999; word-break: break-all',
    'num' => 'font-weight:bold; color:#2563eb',
    'const' => 'font-weight:bold; color:#b45309',
    'str' => 'font-weight:bold; color:#15803d;',
    'note' => 'color:#1E40AF',
    'ref' => 'color:#6E6E6E',
    'public' => 'color:#262626',
    'protected' => 'color:#262626',
    'private' => 'color:#262626',
    'meta' => 'color:#B729D9',
    'key' => 'color:#78350F',
    'index' => 'color:#2563EB',
    'ellipsis' => 'color:#CC7832',
    'ns' => 'user-select:none;',
],

EDIT:

Btw, wouldn't that be a use case where the dumper class could be created with Craft::createObject(HtmlDumper::class) and we could use our own class with random custom styling via dependency injection?

Craft::$container->set(
    HtmlDumper::class,
    fn($container, $params, $config) => new \modules\main\helpers\HtmlDumper($config),
);

@brandonkelly brandonkelly merged commit eafad42 into craftcms:develop Mar 11, 2023
@brandonkelly
Copy link
Member

brandonkelly commented Mar 11, 2023

This alone would have only improved Craft::dump() etc., but not calls to VarDumper::dump() (such as via Collection::dump()).

So I just added a new dumper application component, which is used by Craft::dump() as well as VarDumper::dump(). It incorporates the light theme for web requests, and enables/disables colors based on Controller::isColorEnabled() for CLI requests. (#12869)

@wsydney76 Unfortunately it’s not possible for us to call HtmlDumper::setTheme() from Craft::createObject(), so the DI approach would still not give you full control, as we’d have to call setTheme() after instantiation. However with this new dumper application component, you could override the component from config/app.web.php (not config/app.php):

return [
    'components' => [
        'dumper' => modules\main\helpers\HtmlDumper::class,
    ],
];

@brandonkelly
Copy link
Member

4.4.2 is out with that change.

@DenyEs
Copy link

DenyEs commented Mar 15, 2023

Is there an easy way to go back to "dark" mode style? I find this very hard to read especially on white backgrounds.

@wsydney76
Copy link

@davist11

I find this very hard to read especially on white backgrounds.

True, I personally can't use either the dark or light theme.

The default light theme is quite bad at accessibility, both in terms of font-size and color contrast.

Hope that is on Crafts accessibility todo list, for now you can bring back the dark theme with the method that @brandonkelly pointed out above:

config/app.web.php

<?php

use Symfony\Component\VarDumper\Dumper\HtmlDumper;

return [
    'components' => [
        'dumper' => new HtmlDumper(),
    ],
];

Just uploaded a new version of our starter project, that includes a custom theme that Chromes CSS overview accepts as AAA compliant:

https://github.com/wsydney76/craft4-ddev-starter/blob/main/modules/main/helpers/CustomHtmlDumper.php
https://github.com/wsydney76/craft4-ddev-starter/blob/main/config/app.web.php

@DenyEs
Copy link

DenyEs commented Mar 15, 2023

@davist11

I find this very hard to read especially on white backgrounds.

True, I personally can't use either the dark or light theme.

The default light theme is quite bad at accessibility, both in terms of font-size and color contrast.

Hope that is on Crafts accessibility todo list, for now you can bring back the dark theme with the method that @brandonkelly pointed out above:

config/app.web.php

<?php

use Symfony\Component\VarDumper\Dumper\HtmlDumper;

return [
    'components' => [
        'dumper' => new HtmlDumper(),
    ],
];

Just uploaded a new version of our starter project, that includes a custom theme that Chromes CSS overview accepts as AAA compliant:

https://github.com/wsydney76/craft4-ddev-starter/blob/main/modules/main/helpers/CustomHtmlDumper.php https://github.com/wsydney76/craft4-ddev-starter/blob/main/config/app.web.php

Thank you! I accidentaly wrote it wrong before that's why it didn't work for me. I gave it another go and it works :) To be honest I really like the old "dark" mode style, I can clearly see where my dump is and it a lot nicer to read than a white one. I will have a look at your solution when I will have some more time. Cheers

@engram-design
Copy link
Contributor Author

engram-design commented Mar 18, 2023

If anyone else wants to go back to the colours and style before this, the below is as close as I can get.

image

<?php
namespace modules\sitemodule;

use Symfony\Component\VarDumper\Dumper\HtmlDumper as DefaultHtmlDumper;

class HtmlDumper extends DefaultHtmlDumper
{
    protected static $themes = [
        'light' => [
            'default' => 'color:#007700; background: #fff;',
            'num' => 'color:#0000BB',
            'const' => 'color:#0000BB',
            'str' => 'color:#DD0000;',
            'note' => 'color:#0000BB',
            'ref' => 'color:#FF8000',
            'public' => 'color:#0000BB',
            'protected' => 'color:#0000BB',
            'private' => 'color:#0000BB',
            'meta' => 'color:inherit',
            'key' => 'color:#DD0000;',
            'index' => 'color:#0000BB',
            'ellipsis' => 'color:inherit',
            'ns' => 'color:inherit',
        ],
    ];

    protected function getDumpHeader()
    {
        $header = parent::getDumpHeader();

        $overrideCss = <<<HTML
<style>

pre.sf-dump a[class*="-ref"] {
    background: transparent !important;
    color: #FF8000 !important;
}

pre.sf-dump .sf-dump-ellipsis {
    max-width: none !important;
}

pre.sf-dump .sf-dump-compact,
.sf-dump-str-collapse .sf-dump-str-collapse,
.sf-dump-str-expand .sf-dump-str-expand {
    display: inline;
}

.sf-dump-toggle span {
    display: none !important;
}

</style>

HTML;

        return $header . preg_replace('/\s+/', ' ', $overrideCss);
    }
}

There's some pretty annoying things about their default styling, that we can't really do anything about. They bundle CSS and JS for their additional functionality which isn't really extendable, short of re-writing it all.

There's some custom CSS to override some annoying behaviour

  • Change the gross purple colour when hovering over the "expand" toggle for classes: https://share.cleanshot.com/RT72LNQ0
  • Class truncation is ridiculously small, and honestly just annoying: https://share.cleanshot.com/6504xqLq (set to max-width: 5em; in their CSS)
  • Having to expand nested blocks is annoying. I've enforced everything to be open. I'm sure this is a contentious point, but I find when I'm looking for things, I want to command+F to search.

So while this kinda works, it's starting to feel incredibly hacky.

@engram-design
Copy link
Contributor Author

So having implemented this for a few projects now, it's kinda becoming a bit annoying for me 😅 Not sure if this irks anyone else, but I make use of a lot of Craft::dd() calls, because i'm an ancient debugger.

@brandonkelly are you happy with the default behaviour (using the gnarly styling), and would rather leave it up to developers to extend and modify? Always happy to chat on Discord, and put together a PR.

@brandonkelly
Copy link
Member

@engram-design Not sure I’m following. Do you think we should go back to the dark theme? Or just tweak the light theme a bit? Or drop VarDumper altogether?

@engram-design
Copy link
Contributor Author

@brandonkelly Not suggesting using the dark theme at all, I think it's out of place on a white screen. I don't there's a scenario with dd() will not be shown on a blank white screen. I suppose maybe dump() would though, but either way the default colors are pretty gross.

I can do another PR with the tweaked colours if you like? I didn't think there was a problem with the previous command, but I probably wouldn't suggest getting rid of VarDumper seeing as though you've just switched over.

@brandonkelly
Copy link
Member

@engram-design If you want to PR a nicer color scheme, go for it! I don’t have a strong opinion.

@engram-design
Copy link
Contributor Author

Hmm, if no one else really cares I won't bother with it.

There's just a few edge cases where it's starting to rear its ugly head (like Vue warnings where using {{ dump() }} in template code due to the inline JS it includes). But that's probably just how it is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants