Add config option for more link wrapper

I reviewed/tested this patch and found that while the wrapper was removed, the setting wasn't being saved if I tried to re-enable the wrapper. I tracked this down to the new setting not being added to the plugin definition or defaultSettings array. The updated patch adds these.

Also, I'm not sure it's best to append '-wrap' to the more link class on the wrapper. If you supply multiple classes in the formatter settings, you end up with only the last having the '-wrap' appended, i.e. "btn btn-red" becomes "btn btn-red-wrap".

Here's an updated patch for recent changes in the dev branch.

Re-roll for the latest dev.

Number 4 was totally broken... And to expand on this patch's purpose, it allows you to remove the wrapper added around the more link like this:

-          $project_link['#prefix'] = '<div class="' . $class . '">';
-          $project_link['#suffix'] = '</div>';
+
+          if ($this->getSetting('wrap_more_link')) {
+            $project_link['#prefix'] = '<div class="' . $class . '-wrap">';
+            $project_link['#suffix'] = '</div>';
+          }

Thanks for this. I had the same requirement and this patch fulfilled it nicely (Drupal 8.9.17, smart trim 8.x-1.3)

Patch no longer applies (to 2.0.x branch). Re-roll, anyone?

thanks,
-mike

Patch re-rolled.

I changed the machine name to more_link_wrap to be more consistent with other machine names. I also rearranged things a bit for code clarity.

Depending on if this issue or #3350498: "More link" formatter configuration UI improvements gets committed first, the config element for this change needs to be added to the "More link" details element.

We need to refactor the functional test class once issue this issue, #3350498: "More link" formatter configuration UI improvements, and possibly a few others are committed - lots of opportunities for improving the functional test class (move test node creation to setup()). I'll volunteer to do this because it's making me a bit nutty as it stands, but I don't want to muddy the waters of this task with a pure refactoring task.

Please review.

-mike

I tried to apply patch #9 in Drupal 10.0.5 and PHP 8.1.14, but it doesn't work.

@Yogesh Kushwaha - please provide more information.

Did the patch not apply? Was there a PHP error on the Smart Trim formatter configuration?

The patch will likely only apply to the 2.0.x branch (or the latest 2.0.x-dev version of the module).

-mike

Just re-tested this patch against 2.0.x - still applies cleanly and tests still pass (locally).

-mike

There.. I pushed a commit which requires a re-roll.

Just wondering would this not be better to apply a .twig template file to the more link, which would make it fully themable. This would seem a much more flexible solution than adding a setting for the wrapper.

@lostcarpark - not a bad idea - what exactly did you have in mind for the template file - just the more link, or the more link plus the optional wrapper ("Wrap trimmed content?")?

Ideally, we'd do both, but that could have negative repercussions on folks who don't have the "Wrap trimmed content?" option currently selected unless we put that bit in a {% if %}, which would be a bit wacky (IMHO).

Personally, I'd love to get rid of the "Wrap trimmed content?" option in lieu of this template file approach (which would decrease the number of existing config settings by one AND remove the need for a new config setting for this task), but then we'd have to be opinionated about whether or not the div wrapper is included by default in the base template file.

If we do just a template file for the more link, that's a simpler solution, but IMHO less elegant.

Thoughts?
-mike

I would definitely favour removing the "wrap trimmed content" setting and putting everything in the template, as it gives users maximum flexibility, and would make the summary fully themable.

However, this could have unexpected consequences for existing users, depending on whether the wrapper is in the template by default (I think the default template should match the current default, which I think is not wrapped). We could either provide an alternative template with wrapping so users could just copy their preferred template into their theme, or we should just put the instructions for a wrapper in the comments of the .twig file.

If we were to take the route of adding a .twig file, I think the important decision to make would be whether the template should cover just the More link, or should it cover the output of the whole trimmed output.

If the template just covers the More link, it has the advantage that we wouldn't be affecting the existing setup at all, we just wouldn't need to add the new checkbox the OP proposed.

If it covers the whole trimmed output, it would have the advantage of making the Smart Trim output fully themable, and would offer a lot more flexibility, but would have the disadvantage that people currently using the "wrap trimmed content" checkbox would need to modify their .twig file.

Perhaps a solution to the "wrap trimmed content" issue would be to keep the setting for now, and make the wrapper a conditional element in the .twig file, but to put a warning on the settings, shown if the "wrap trimmed content" option is checked, with the message: "This option is no longer recommended in will be removed in version 3.0 of the module. Please use the .twig template instead."

I have checked in an initial cut of moving the output display to a template.

It actually required only minor changes to the viewElements method.

I added a smart_trim_theme hook to the .module file, and the template file, smart-trim.html.twig.

I also added a smart_trim_theme_suggestions_smart_trim_alter hook, so you can have specific themes for entity type and field name, for example, smart-trim--node--body.html.twig. Ideally, I'd like to also include the bundle name and view mode in here, but I haven't managed to figure them out.

Somewhat to my surprise, all tests pass without change. However, the HTML output should be the same as before, so perhaps I shouldn't be surprised.

I've marked "Needs review", because I'd like some feedback, but I think some more work will be needed. I haven't tested translation works correctly. I think some additional tests would be wise. I'm not sure what tests are possible around templates, so I'll look into that.

Note that if #3350498: "More link" formatter configuration UI improvements gets merged ahead of this, a rebase and some refactoring will be required.

@lostcarpark - looks really good - I made some requests (mainly about variable names) as you can see above.

As far as tests, I think we could probably add a couple of new functional test methods

1. One focused on the overall wrapper class - if it is set, make sure it is present (and vice-versa).
2. One focused on the more wrapper class - again, if is set, make sure it is present (and vice-versa).

Thoughts?
-mike

Thanks Mike,

Those are all good corrections. I don't know why I always type "entiry" when I mean "entity".

Those sound like good test suggestions. Both should be fairly easy to test.

The test of the More link wrapper might make sense to add to the testMoreLink() test, which is part of #3350498: "More link" formatter configuration UI improvements.

@lostcarpark - thanks for putting up with all my nitpicks! I added a few more for you 😀

-mike

I have made the requested changes.

I've also found how to get the bundle name, so I've added that to the suggestions. I've added a more general function to get combinations of suggestions.

I would really like to add the Display Mode to the suggestions, as having multiple display modes for the same content type, and wanting to use different templates for each, seems very feasible. So far I haven't found a way of getting the Display Mode in the viewElements function, so if anyone has any suggestions, I'd love to hear them.

@lostcarpark Looking really good!

For the "looking for the bundle in viewElements()" bit, I'm pretty sure I went down that rabbit hole before and it's a no-go. As an example, see web/core/modules/comment/src/Plugin/Field/FieldFormatter/CommentDefaultFormatter.php - this formatter has a separate setting for the bundle, which implies it isn't available.

-mike

@ultimike, did you mean the display mode rather than the bundle? I think I'm okay for the bundle.

@lostcarpark,

Yikes! Indeed I did. My comment #26 should read:

For the "looking for the view mode in viewElements()" bit, I'm pretty sure I went down that rabbit hole before and it's a no-go.

-mike

I've added tests for the trimmed text wrapper, and for the more link wrapper.

I've also added a unit test for the template suggestions.

I have rebased to include recent commits.

Wondering should we make the "Wrap trimmed content" options in an expanding box like the More link options in #3350498: "More link" formatter configuration UI improvements?

I went ahead and rebased 2.0.x on to this branch with @lostcarpark (and others) watching - we had to resolve a number of code conflicts along the way.

There are some minor issues with the code that will be easily resolved in the near future.

Currently, tests will not pass! 🧙🏼‍♀️🚫

-mike

Fixed a couple of lines duplicated during the merge, and fixed the testWrapperClass test, which needed updating for the schema change.

Considering whether a test can be added to test the twig template.

Added a new test that uses a test theme with overrides to the smart-trim.html.twig file for specific fields.

Tests using the default template for Body field, and a wrapped field and an unwrapped field with custom themes.

All tests are now passing. 🧙💫

I'm good with these changes - and the changes to the config UI. I _would_ say that we should be the wrap class stuff in a details box (just like the "More" link stuff), but as it is deprecated, I'm not sure it is worth the effort.

This is a really nice addition to the module (the twig template stuff) and all the new tests make me very happy.

-mike

I agree the wrapping the class stuff would be a nice to have, but my feeling is there's a lot going on already, so it wouldn't be wise to also add a schema change on top of it. My gut feeling is that we should open an issue for it, and if someone wants to take it on, it should be a cookie cutter copy of the More link wrapper. The test for it should be able to use the same database dump.

If we get to a 3.0 release before anyone picks it up, it could be closed.

Thanks for all the help. Merged.