-
Notifications
You must be signed in to change notification settings - Fork 11.6k
[12.x] Add @maybe
Blade directive for conditional HTML attributes
#57235
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
base: 12.x
Are you sure you want to change the base?
Conversation
To me, the naming is not intuitive. I would call it |
I appreciate the feedback, but I'll leave the naming to Taylor. |
It'd be fine with |
Good one! One problem is the naming, I think |
I updated the naming section of the PR description. |
Thanks a lot 👍🏻 |
I think this should mirror
|
It is different in the sense that the For this use case here it makes it longer by 6 chars for the majority of situations where we have one data attribute. Don't really like it because the reason for the PR is to make things less verbose. That said, I'll have a look if we can support both. Edit: Happy to implement. For now I'll leave it to Taylor to decide first. {{-- Attributes: 1 --}}
<a @if($link->title) title="{{ $link->title }} @endif">
<a @maybe('title', $link->title)>
<a @maybe(['title' => $link->title])>
{{-- Attributes: 2 --}}
<a @if($link->title) title="{{ $link->title }} @endif" @if($link->rel) rel="{{ $link->rel }} @endif>
<a @maybe('title', $link->title) @maybe('rel', $link->rel)>
<a @maybe(['title' => $link->title, 'rel' => $link->rel])>
{{-- Attributes: 3 --}}
<a @if($link->title) title="{{ $link->title }} @endif" @if($link->rel) rel="{{ $link->rel }} @endif @if($link->clickId) data-tracker="{{ $link->clickId }} @endif">
<a @maybe('title', $link->title) @maybe('rel', $link->rel) @maybe('data-tracker', $link->clickId)>
<a @maybe(['title' => $link->title, 'rel' => $link->rel, 'data-tracker' => $link->clickId])> |
fyi, previous unsuccessful attempt at |
Similar. Mine is minus the complexity. Though, apparently high demand for a solution. |
I tried using This directive is quite valid. It could be expanded to include conditions like the |
I haven't dug deep into how this renders attributes, but I would expect the following to happen for these different attribute types. [
'crossorigin', // crossorigin
'data-persistent-across-pages' => 'YES', // data-persistent-across-pages="YES"
'remove-me' => false, // [removed]
'keep-me' => true, // keep-me
'null' => null, // [removed]
'empty-string' => '', // empty-string=""
'spaced-string' => ' ', // empty-string=" "
'zero' => 0, // zero="0"
'one' => 1, // zero="1"
]; <div
crossorigin
data-persistent-across-pages="YES"
keep-me
empty-string=""
spaced-string=" "
zero="0"
one="1"
/> This will keep it inline with how Vite handles attribute types and values. |
See arbitrary attributes in #43442 for more on these decisions. |
Hey Tim! I already read that in your comment to the PR linked above. But I kindly disagree. Personally I don't care what Vite does, what I care about is how I can add my own attributes in a non-verbose way. This PR seeks to handle the majority case we deal with every single day, not to be "aligned" with the Differences:
As I mentioned in my PR description, I would name this Both concepts are valid, but they cannot be merged in one (one wants to render false, one doesn't). Hence, what you are asking for is unfortunately nothing for this PR. 🙏 |
Appreciate you pushing back, @NickSdot! Always appreciated. To clarify, when I say Vite, I mean what we do in Laravel itself. Having different attribute rendering mechanics for two Laravel features seems like a footgun. But even taking the stand that we don't want to be inline with Laravel's attribute handling in the Vite space, I would still push back on the current rendering proposal. If nothing else, we should respect HTML itself. It explicitly mentions that ![]() |
I think my brain goes to tooling like jQuery when I see that we don't render empty strings and whitespace only strings. I probably need to think on that some more to come up with a compelling argument as my brain is deep in other stuff right now. None of this is a hill I wanna die on, btw. Just sharing a perspective and prior art in Laravel related to the feature to ensure we keep the framework cohesive when and where it makes sense. |
@timacdonald well, as I mentioned above, there is no way to satisfy two completely contrary concepts in one solution. Below I make a point why both are valid (+ spec conform) and why we need two mechanisms.
I knew that this will be the next argument. :) For the follwing reasons I need to push back again: 1) There are more than one relevant spec here. The ARIA spec for instance. Accessibility APIs expect explicit tokens. E.g:
2) Only valid for presence indicators (boolean attributes) What you are quoting is a separate concept with explicit behaviour. This, however, does not mean that I cannot use true/false in enumerated attributes (next section after the one on your screenshot: 2.3.3 Keywords and enumerated attributes). True/False are not forbidden values for enumerated attributes. We can do whatever we want, just cannot expect "boolean attributes" behaviour from it. To bring a simple example; you will agree that the folowing is neater foo.active = foo.active === 'true' ? 'false' : 'true'; than if (foo.hasAttribute('data-active')) {
foo.removeAttribute('data-active');
} else {
foo.setAttribute('data-active', '');
} Enumerated attributes allow us excactly that. And here we are also back to the previous point: ARIA attributes are enumerated attributes, not boolean ones. Both ways are HTML spec conform, even if we ignore that ARIA is a separate spec. 3) Third Party Expectations I am all for following specs. And I also have proven above that we are aligned with the spec. However, I still would like to throw in third party stuff. If a third party expects explicit
Yes, again, I don't object. Both concepts have their place. That's why we need two solutions for it. I believe we shouldn't try too hard to unify something that cannot be unified. It's like in the real world, we have a Slotted screwdriver and a Phillips screwdriver. Both aren't footguns, but solutions for different problems. 4) Last but not least You get the point: this is not only for custom data attributes.
Unfortunately, it (subjectively) feels like Taylor tends to close PRs when you hop in with objections. So if your objections are not fully thougt out... it's demotivating to be closed just because. No offense, of course! ❤️ I hope my arguments above are compelling enough to "book a win" here. |
No offense taken. It is my job to offer opinions here and there. Sorry if I've put you off here or on other PRs. I can see utility in a feature of this shape. FWIW, I hope some form of this gets merged. |
No, it's also not like that. Wasn't put off myself. So far you luckily been supportive to all my PRs. Sorry if I didn't express myself clear enough. 🙏 |
Awesome! |
I fully agree with @timacdonald and would rather see a single |
Adding to @willrowe's comment, the @attrs([
'aria-hidden' => $hidden ? 'true' : 'false',
'aria-expanded' => $expanded ? 'true' : 'false',
]) One can easily add a helper to their code base if wanted. Or just use @attr([
// json_encode will output booleans, numbers and null as a unquoted strings
'aria-hidden' => json_encode($hidden),
'aria-expanded' => json_encode($expanded),
]) But attribute toggling, as @timacdonald described, would be very awkward to accomplish with the current proposal. Also, calling the directive Mind that Blade's directives can also be used for non-HTML content, like markdown emails and Envoy tasks. I'd prefer, if added, for it to be called something like If different behavior due to Insert "Why not both?" meme here We could add a A |
This is Blade. Just saying. Y'all keep discussing things that this PR doesn't seek to solve. As we now already know there must be two different solutions because both concepts are diametrical to each other. This isn't "tailored" to one persons requirements, this is the majority use case. We all set title and rel attributes all the time. The alternative examples proposed above are hilarious. You realise that they are longer than writing the actual control flows this PR attempts to get rid off? About naming, I repeat, I leave that to Taylor. Guys, keep on bike shedding unrelated stuff instead of working on a complementing PR to add the other missing piece. I am sure that's how we will get good things! ✌️❤️ Edit: Edit 2:
|
Of course not. Those would be covered in the behavior everyone would expect a With sane rendering rules that follow the HTML spec, minus
Sure, mate. It is such an odd case it got a custom directive, a wrapper class, and a section on docs. https://laravel.com/docs/12.x/blade#rendering-json
Yes, it is. Or at least when one prioritizes The gripe is not on the value of the directive, as I, and I am sure others who commented out, like the proposed shorthand syntax in general. The gripe is on the proposed esoteric rendering rules. This argument doesn't make any sense on the But whatever, you do you. Good luck with your PR. I like the idea, just not the oddities, such as treating booleans as strings (which is perplexing). If not merged, consider publishing it as a package. I am sure many other developers would benefit from it for Have a nice day =) |
I answered the HTML spec question above in detail. This is spec conform. Read up enumeration attributes instead of ignoring it and liking comments which also got it wrong. Enumeration example from ARIA: Enumeration example from app: These have nothing to do with boolean attributes. Unfortunately, I cannot do more to help you to understand the difference.
Thanks for making the argument for getting this merged. And yet you don't want to see it merged. Because it isn't tailored to your use case? ;) I wish you the same! |
I do want to see it merged. Just not with such esoteric rendering rules. I find the syntax great: <a href="..." @attr('title', $title)>{{ $label }}</a> Where the The proposed syntax is very handy. Just not your particular use case for rendering attribute values in such a manner no one would expect.
Not my use case. HTML attributes spec. Imagine someone using Web Components opening issue after issue as they expect boolean attributes to behave conforming to the spec. Enumeration attributes are another spec suitable to specific use cases. It is not the general use case. And as such, IMO, it could be subject to a future addition, as I believe the general use case would benefit more developers. Or even provided by a 3rd-party package. I am sorry. I won't spend more of my time trying to help you bring this addition to the framework. I wish you the best luck. |
Has anyone tried building this as a simple package that registers the directive as a Blade extension? I think I do find the <div class="something something-else" @attribute('wire:poll', $shouldPoll)>
...
</div> |
Not as a package, but I have it in service providers in the hope to get rid of it when this gets merged. The attribute you mention, by spec, is an enumeration attribute because it also allows other values like You probably didn't read all the comments here. My personal conclusion was that we need Think of ARIA attributes like By adding |
I definitely think |
How would you feel about calling this one |
i just wanted to drop a comment because i really like this PR. i think Hot take: personally, i would actually prefer if blade components stopped using That way, |
I'd like to offer some feedback that might help move this forward: The Good
Considerations
SuggestionWhat if we had two complementary directives:
This would give us the best of both worlds without forcing one behavior to handle incompatible use cases. Overall, I think this feature would be valuable for the framework. The key is finding the right balance between convenience and predictability. Keep up the great work!
|
More than one person already disagreed with the naming being intuitive (you have addressed that further down 👍)
I'd assume that Taylor is less likely to merge this since he then can simply refer to an existing package. |
Hello @NickSdot I have read all the comments here and I think the current state of the PR is fine as it is, including the directive name. |
@imhayatunnabi, I appreciate the productive feedback. 1) Naming & GeneralI agree with you that " Why: given what you propose I would expect that the following directives would not required us to prefix with data/aria like so:
Which makes them unusable for everything else. This means for everything non-data and non-aria, potentially vendored, we still would need to use So what would we win? We still have the problem that we need solutions for different use cases, like rendering enumeration attributes that require 2) Package FirstI am really having a hard time to agree that this should be package functionality. These are such common cases that I actually believe it should be explicitly discouraged to have them in packages. a) we will end up with several community implementations that work slightly different. Each time you work on something you first must find out which flavour is used. b) if Laravel ever will have it in core, ecosystem implementations will break. In fact one of the reasons why I prefer to not have it in a service provider. I cannot help but believe it's a bad idea. Curren StatusWe have 10 positive reactions in the form of emojis, 4 positive reactions via comment. There are only 3 negative reactions with concerns regarding the functionality. These 3 could and should be addressed with a second implementation, IMHO. Some more comments have concerns only with the name. I understand that If anyone has naming ideas please step forward. |
Yeah, |
Fair. But what are our options? Taylor wants it less broad, so everything from You proposed
I had Unfortunately, I am really not sure how "too broad" can be avoided here. Any other ideas? |
what about something like |
I still do not understand why you cannot just pass |
Sure. Similar to |
Because it is convenient for a feature that we use all the time. The whole promise of Laravel is to make things more convenient. So, when an easy to understand helper can safe us from casting things all the time I believe it should do so. You actually make the point for it yourself, because as you correctly note it is always a string what we want. Taylor seems to agree. As far as I understand this is about naming now. |
I think the main issue here is that if I'm using the directive like this: @php
$opt_in = false;
@endphp
<input @maybe('checked', $opt_in) name="opt_in" type="checkbox" /> I would expect it to render this HTML: <input name="opt_in" type="checkbox" /> However, I would get this instead: <input checked="false" name="opt_in" type="checkbox" /> Which is the opposite of what I want. There is a |
You again repeat the same point that was discussed in length. Yes, your expectation is valid. Yes, my expectation is valid. That's why there is no way to get two diametrical different functionalities into one directive. It's impossible. That's why we need two directives to cover all cases. I don't understand how that is not absolutely obvious to everyone at this point. Additionally, and ignoring that we already have all the boolean directives, what is the whole point of what you want to do here?
You are literally making an argument for a problem that doesn't exist for boolean attributes. This PR solves a verbosity problem that we don't have with boolean attributes. But you want to "unsolve" that by making dealing with the actual verbose attributes more complicated by forcing us to cast every time manually. Please tell me that you realise how little sense this makes. |
Hello @NickSdot what about something like this: if (in_array($parts[0], ['checked', 'selected', 'disabled', 'required', 'readonly'])) {
// Throw an exception...
} |
For the built-in booleans this would make things perfectly clear. But different story for vendor and custom booleans. Not sure if we really need to babysit to such an extend. HTML itself doesn't prevent us from writing broken HTML, though. Two people here act like this is a massively complicated directive while it's not. It basically mirrors the exact behaviour of the Probably it's just good as is, but if Taylor wants me to add it I can do that of course. |
To be honest, I think the real discussion here shouldn’t be about the feature itself, but rather about its wording or naming. If everyone reads through the entire pull request and its comments, it should be crystal clear that we do need this — as well as the alternative solution — since there are two valid use cases. Both currently lead to bloated and messy code, with multiple Maybe all of the negative comments could have been avoided if this PR had included the other helper as well - but then again, that might have made the PR too large to implement in one go. |
This PR adds a new
@maybe
directive that conditionally renders HTML attributes with values, complementing the existing boolean attribute directives like@checked
,@selected
,@disabled
,@required
, and@readonly
.Problem
While we have directives for boolean attributes, we still need verbose
@if ... @endif
blocks for attributes with values:We cannot keep adding specific directives for every possible attribute, so we need a dynamic solution.
Solution
The
@maybe
directive renders an attribute with a value only when the value is notnull
, not an empty string, and not whitespace-only:Before/After
Behaviour Matrix
The directive intentionally differs from a simple
@if()
check by treating0
andfalse
as valid values, since these are common in data attributes for counts, flags, and boolean states.'foo'
data-attribute="foo"
0
data-attribute="0"
false
data-attribute="false"
true
data-attribute="true"
''
null
' '
Naming
I considered several alternatives:
@when
(too generic, likely better for other future use cases),@flag
(implies boolean values only, whereas this handles strings, numbers, bools),@attribute
and@optional
(too long),@attr
and@set
(don’t make the conditional nature clear).@has
was tempting as it reads well: “has$title
, then rendertitle
”. However, the parameter order would need reversing to@has($title, 'title')
, which breaks the pattern of other Blade directives where the static value comes first.@opt
is appealingly terse but perhaps too cryptic.@maybe
has the right balance. It’s short, clearly conditional, and reads naturally with the attribute name first: “maybe render title if$title
”.