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

[8.x] @class Blade directive #38016

Merged
merged 7 commits into from
Jul 15, 2021
Merged

[8.x] @class Blade directive #38016

merged 7 commits into from
Jul 15, 2021

Conversation

danharrin
Copy link
Contributor

@danharrin danharrin commented Jul 15, 2021

Hey! 👋

Conditional class syntax for Blade component attribute bags has been available in Laravel since v8.27.0. It can be used to conditionally merge attributes into the bag based on Boolean expressions, like so:

<span {{ $attributes->class([
    'text-sm',
    'font-bold' => $active,
]) }}>
    Dashboard
</span>

In this example, the font-bold class would be merged into this class list when the component is $active.

I would like to expand the scope of this syntax one step further, and make it available in a @class Blade directive. Similar functionality can be found in many frontend frameworks, including Vue.js and Alpine.js. This will allow Blade developers to use conditional classes on any HTML element, not just when merging them into the attribute bag. For example:

<div>
    <span @class([
        'text-sm',
        'font-bold' => $active,
    ])>
        Dashboard
    </span>

    <svg @class([
        'w-6 h-6',
        'text-indigo-600' => $active,
    ])>
        <path />
        <path />
        <path />
    </svg>
</div>

To keep this new directive DRY, I extracted the class array compilation logic into a new array helper, Arr::toCssClasses(). Let me know if there is a better place to put this logic!

Alternative

Alternatively, I could remove the @class Blade directive and allow developers to use the raw Arr::toCssClasses() helper, like so:

<div>
    <span class="{{ Arr::toCssClasses([
        'text-sm',
        'font-bold' => $active,
    ]) }}">
        Dashboard
    </span>

    <svg class="{{ Arr::toCssClasses([
        'w-6 h-6',
        'text-indigo-600' => $active,
    ]) }}">
        <path />
        <path />
        <path />
    </svg>
</div>

* @param array $array
* @return string
*/
public static function toClasses($array)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the method does stay here, it toCssClasses may be a better name so the context of what the method does is clear when the Arr class is used outside Blade.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah it was a bit of a struggle to find an appropriate name, toCssClasses sounds good but lets see what the Laravel team think 👍

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't toHtmlClasses be more appropriate? The point is that this is in the context of view templates, but it's not related to styling necessarily. You could have @class(['clickable' => $enabled])to control some JavaScript logic for instance.

@taylorotwell taylorotwell merged commit c5908c5 into laravel:8.x Jul 15, 2021
@stancl
Copy link
Contributor

stancl commented Jul 15, 2021

@taylorotwell What do you think about the conversation above suggesting the use of toHtmlClasses instead of toCssClasses? Because the CSS assumption is kind of biased when classes are used for many many things outside of styling.

@danharrin danharrin deleted the feature/class-blade-directive branch July 15, 2021 14:19
@danharrin
Copy link
Contributor Author

danharrin commented Jul 15, 2021

I'll open a PR for the docs 🎉

Update: laravel/docs#7194

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

4 participants