Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Problem
-
In order to output a link as a button, code has to manually specify an array of classes, which
- means duplicate code
- can become inconsistent
- is borderline impossible to detect + alter for themes.
Proposed solution
-
Allow a #type link to have an optional #button_type, so that the following works:
array( '#type' => 'link', '#title' => 'Delete', '#button_type' => 'danger', );
| Comment | File | Size | Author |
|---|---|---|---|
| #18 | interdiff.txt | 39 bytes | marvil07 |
| #18 | add_button_type-2243575-18.patch | 7.92 KB | marvil07 |
Comments
Comment #1
sunWorking implementation, including proper test coverage.
Comment #2
sunComment #3
sun2: link.button.2.patch queued for re-testing.
Comment #5
sunMerged HEAD
Comment #6
marvil07 CreditAttribution: marvil07 commentedAfter this patch was created a new class get into D8, RenderTest, I guess we want to reuse it.
Comment #7
joelpittetI"m not sure this is inline with the classy theme to move classes to the classy theme and remove them from stark. I'll send @mortendk and @davidhernandez over here to check into this.
Comment #8
joelpittetIs there a prevision for the case of not wanting any classes on buttons? Or removing the 'button' class?
Comment #9
mortendk CreditAttribution: mortendk commentedooooh these prerendered classes are a pita to work with later on & one of the things that have haunted themers down when they wanted to add in other frameworks, that lets say used the
.buttonfor something specific (and you dont wanna maintain your own version of that framework offcourse)Heres an idea - what if we prefix that kinda stuff with, so its clear where it comes from, and dont end up taking common classnames (yes i know im anal about this)
drupal-button ?
drupal-button--
By at least using some kind of name spacing we know where it comes from & the chance of it colliding with an exisiting css framework is small.
What i would Looooooooove to see was instead
button.html.twigand used addClass(' button--mucho-danger') instead, that would make it clear for the themer where to fix this class thingieComment #10
davidhernandezYeah, I was going to say the exact same thing as Morten. Not the class prefix part ;) but the template part. We are trying to eliminate, as much as possible, classes being generated, so I don't think we want to add more. The problem is how we are styling/creating buttons in core. It would be much nicer if we had a button template, and pass a variable of the button type to it. Then themers could deal with it as they want, including changing how the markup works.
Comment #11
marvil07 CreditAttribution: marvil07 commentedI was wrong about RenderTest, it's not a base test as the one added here.
Here an updated patch, based on what sun proposed, creates a twig template for links, which is needed in order to add classes on the template.
I modified the implementation to follow several drupal changes like link theme variables.
Also I'm using it on the LinkGenerator class.
Notes:
Comment #13
mortendk CreditAttribution: mortendk commented@marvil yeah for templates ! - could we call it
button-link.html.twigin that its clear for the themer that dont know anything what the template might is about.Comment #14
joelpittetWhat about just
link--button-danger.html.twigand just add to the template suggestions of the existing template?Comment #15
marvil07 CreditAttribution: marvil07 commentedA new patch, here I'm trying to use theme suggestions to pick either link.html.twig or link--button.html.twig depending on the use of the new '#button_type' property, BTW since logic is now in php we can again differentiate between isset() and empty() cases, which seems to be not possible in twig.
In any case, I'm not sure if I'm using theme suggestions correctly, since they seem to be picked up, but the template in use is always link.html.twig, any suggestion is welcome.
Comment #16
marvil07 CreditAttribution: marvil07 commentedI forgot the interdiff
Comment #18
marvil07 CreditAttribution: marvil07 commentedKernelTestBase already have a render(), reuse it, question about how to use twig templates is still open.
Comment #22
marvil07 CreditAttribution: marvil07 commentedActually use \Drupal\Tests\Core\Render\RendererTestBase
Comment #23
tim.plunkettThis should be injected.
This can be removed
public function
Why is this redclaring assertRaw and friends? Please use AssertContentTrait.