themeum / kirki

Extending the customizer
https://kirki.org
MIT License
1.26k stars 327 forks source link

css_vars font-family bug. #2246

Closed greathemes closed 5 years ago

greathemes commented 5 years ago

Hello. I found one error. When we use css_vars with font-family property, the result is bad or I'm doing something wrong.

Code:

    Kirki::add_field( 'id', [
        'type'        => 'typography',
        'settings'    => 'setting_id',
        'label'       => 'text',
        'section'     => 'section_id',
        'default'     => [
            'variant'     => 'regular',
            'font-family' => 'Playfair Display',
        ],
        'transport' => 'postMessage',
        'choices'   => [
            'fonts' => [
                'google' => [ 'popularity', 30 ],
            ],
            'variant' => [ '300', '300italic', 'regular', 'italic', '600', '700' ],
        ],
        'css_vars' => [
            [ '--heading-font-family', '$', 'font-family' ],
        ],
    ] );

Suppose I chose Ubuntu font in Customizer.

Expected result: <style id="kirki-css-vars">:root{--heading-font-family:Ubuntu;}</style>

Current result: <style id="kirki-css-vars">:root{--heading-font-family:Ubuntu, ;}</style>

It adds a comma to the end of the font name. The question is whether there should be something else after this comma and I am doing something wrong or there should not be a comma at all (I think there should not be).

Greetings.

aristath commented 5 years ago

Are you using the stable version from wordpress.org, or did you download/clone from this repository?

greathemes commented 5 years ago

Hello, I'm using the wordpress.org version. The important point is that the default font is rendered correctly. Only when we save it then it adds this comma and it makes an error.

aristath commented 5 years ago

I believe I know where the issue is... Marking this as a bug and I'll try to get this fixed during the weekend and release an update 👍

greathemes commented 5 years ago

It would be great because I'm slowly finishing the theme on themeforest and unfortunately I won't move on with this error :) Or otherwise, I'll move on but I will have to look for workaround and it is known that it's best to use native functions :)

Thank you!

Btw: Kirki is amazing :)

aristath commented 5 years ago

As a quick fix you can try this:

  1. set transport to auto instead of postMessage
  2. Use the output argument instead of css_vars. So basically delete the css_vars line you have there, and instead add this:
    'output' => [
    [
        'element'  => ':root',
        'property' => '--heading-font-family',
        'choice'   => 'font-family',
    ],
    ]

    I'm not sure if it will fix the issue, but you can try it. If you're in a rush and you're embedding Kirki in your theme, then in your embedded version you can edit the modules/css/property/class-kirki-output-property-font-family.php file, and comment-out (or just delete) lines 43-56: https://github.com/aristath/kirki/blob/v3.0.44/modules/css/property/class-kirki-output-property-font-family.php#L43-L56 You mentioned you're using the stable version from wordpress.org, so in v3.0.44 (the latest stable version) these are the lines that are causing the problem.

greathemes commented 5 years ago

Thank you very much :) I will try it tomorrow :)

greathemes commented 5 years ago

I found another small bug :) Let's take the example you once wrote somewhere

'css_vars'        => array(
    array( '--text-font-family', '$', 'font-family' ),
    array( '--text-font-weight', '$', 'variant' ),
    array( '--text-font-size', '$', 'font-size' ),
    array( '--text-line-height', '$', 'line-height' ),
),

Apart from the font-family error in this topic, font-weight does not work if you choose regular. It doesn't work then, because it returns 'regular' string ( --text-font-weight:regular; ). Of course, then it doesn't work because in font-weight we can only enter 400 or 'normal', not 'regular'.

I hope this is also a small bug that you can easily fix in the next update :)

Greetings.

greathemes commented 5 years ago

Hi :) I would not like to be annoying, but do you see any chance that you will be able to upload update this week? :) I just want to know whether to wait or try something my own way? :)

greathemes commented 5 years ago

@aristath - Thank you for your commits, unfortunately I don't know very much what to do now, I'm not so good at php. I looked at what you put in and replaced it in my local version. Unfortunately it didn't help, so I probably did something wrong? Should this work for version from Wordpress.org or from Github? And there I also saw the file test-kirki-fonts-php, maybe I should test something with it? But how?

Sorry, I'm not very experienced with plugins yet :)

greathemes commented 5 years ago

@aristath - Hey, I made a small change to the code (class-kirki-modules-css-vars.php):

if ( isset( $args['type'] ) && in_array( $args['type'], array( 'typography', 'kirki-typography' ), true ) ) {
    if ( isset( $val['font-weight'] ) && 'regular' === $val['font-weight'] ) {
        $val['font-weight'] = '400';
    }
}

I replaced it with:

if ( isset( $args['type'] ) && in_array( $args['type'], array( 'typography', 'kirki-typography' ), true ) ) {
    if ( isset( $val['variant'] ) && 'regular' === $val['variant'] ) {
        $val['variant'] = '400';
    }
}

So not font-weight but variant. From what I'm testing, this fixes the css-vars problem but I will test some more. However, I am still looking for a font-family with it, there is still an error with this comma.

greathemes commented 5 years ago

@aristath - The comma appears at this point:

File: class-kirki-values Function name: typography_field_tweaks() Line: 50.

$value['font-family'] .= ', ' . $value['font-backup'];

However, I am not sure if this comma can simply be deleted? Anyway, I hope it's a good direction for you :) Just remember that I'm talking about CSS variables all the time, "output" works fine. I would not like us to accidentally fix one and destroy the other :D

aristath commented 5 years ago

These lines: https://github.com/aristath/kirki/blob/d502fcf7a50a0663cb316721bc9e6479fdf1050c/core/class-kirki-values.php#L48-L52 replace them with this: https://github.com/aristath/kirki/blob/20ce2a6d8184f333b0a72a78141cf2183ca02fe3/core/class-kirki-values.php#L48-L55 The fix has been pushed in this branch: https://github.com/aristath/kirki/tree/3045 so you can just take the file from there if you want 👍

greathemes commented 5 years ago

Thank you :) And out of curiosity, when are you planning this update officially at wordpress.org?

aristath commented 5 years ago

Thank you :) And out of curiosity, when are you planning this update officially at wordpress.org?

As soon as there's at least another fix in there. Right now v3.0.45 only includes the fix for this ticket - in other words a fix for an issue that nobody else has ever reported, so chances are it has something to do with your specific setup. This change is minor and won't affect anyone but you. I'd like to add something else in there too, so I'm hoping I'll add some other bugfixes this weekend and release a more complete update. 👍

greathemes commented 5 years ago

Oki, thanks :)