PLATTA-4940 use twig for link title only when needed #119
Conversation
|
Kudos, SonarCloud Quality Gate passed!
|
There was a problem hiding this comment.
You should check this with our frontend team (Mikko / Tero / Markus etc.) because I'm not 100% sure if we want to do this since we'd have to maintain two different templates for links. This feature was originally done so ALL links are run through same template.
Link filter might need an exception too: https://github.com/City-of-Helsinki/drupal-module-helfi-api-base/blob/main/src/Plugin/Filter/LinkConverter.php
I'm wondering if we could just exit early if the link has a route or somehow check if the link is internal, like
if ($url->isRouted()) {
return parent::preRenderLink($element);
}since they cannot be external anyways.
src/Link/LinkProcessor.php
Outdated
| @@ -35,7 +34,30 @@ public static function preRenderLink($element) : array { | |||
| $element['#attributes']['data-protocol'] = $scheme; | |||
| } | |||
| } | |||
| $element['#title']['#attributes'] = $element['#attributes'] ?? []; | |||
|
|
|||
| if (!empty($element['#attributes']['data-is-external'])) { | |||
There was a problem hiding this comment.
I think this can be simplified by moving $use_helfi_link = TRUE inside previous if condition:
if ($resolver->isExternal($element['#url'])) {
$element['#attributes']['data-is-external'] = 'true';
if ($scheme = $resolver->getProtocol($element['#url'])) {
$element['#attributes']['data-protocol'] = $scheme;
}
$use_helfi_link = TRUE;
}There was a problem hiding this comment.
@tuutti yes, but I wasn't sure if there's any case where the $element['#attributes'] 'data-protocol' or 'data-is-external' could come already populated from the render array for an internal link !? cause basically if that's the case, then twig conditions would already cover that case...
I can do it like that if you think it should not be the case that an internal link could have these data- attributes
src/Link/LinkProcessor.php
Outdated
| /** @var \Drupal\Core\Url $url */ | ||
| $url = $element['#url']; | ||
| $url_attributes = $url->getOption('attributes'); | ||
| if (!empty($url_attributes['data-selected-icon']) |
There was a problem hiding this comment.
Is this condition necessary? I think data-selected-icon it's set in https://github.com/City-of-Helsinki/drupal-hdbt/blob/main/templates/module/helfi_api_base/helfi-link.html.twig#L4 and the template is only used when the link is rendered through this preprocess.
EDIT: Nevermind.
@tuutti the isRouted would not cover the internal links that have "data-selected-icon" |
|
@dragos-dumi-ibm One option would be to change this to be opt-in rather than apply to all links by default. I'll look into this today and let you know if it's possible. |
|
@tuutti do you have any suggestions / updates on this opt-in? the idea is good one, although I'm bit concerned on the impact on each site, if there's something more than the 'base' in certain ones. |
|
@dragos-dumi-ibm not yet. I didn't actually have to time to investigate this before my vacation. I'll get back to this at some point next week. |
|
@dragos-dumi-ibm Looks like we have a pretty good control over code using this feature. Here's some features using this:
We can either add a flag to Url options to indicate whether the url should be processed, or remove the preprocessor altogether and run it manually against those feature. |
|
@tuutti
Can't really tell which one is best:
Or another option - not sure if this is what you had in mind. To keep what is currently in this PR, just in addition to only do the external / icon checks we are doing in preRender for the 'opt-in' / 'flagged' links ? |
|
@dragos-dumi-ibm correct me if I'm wrong, but I think the icon logic is only used in formatted text fields (CKEditor link plugin):
We could probably remove the icon logic here just by explicitly running the link through EDIT: Looks like it's used in Banner paragraph too. |
|
@tuutti |
|
@dragos-dumi-ibm Hmm, my only problem with the current approach is that we have to add an exception for every new feature using this. Maybe we could just change the Then just change the |
|
@tuutti I've made some changes. |
|
@dragos-dumi-ibm OK, I had time to actually test this yesterday and I now get why you've done this the way you did it originally. Should've actually tested this before to not to waste your time, sorry! I think we have three options here:
I did some rudimentary testing with this: public static function preRenderLink($element) : array {
$useTemplate = FALSE;
if (isset($element['#url']) && $element['#url'] instanceof Url) {
$url = $element['#url'];
if ($element['#title'] instanceof Markup) {
$useTemplate = TRUE;
}
/** @var \Drupal\helfi_api_base\Link\InternalDomainResolver $resolver */
$resolver = \Drupal::service('helfi_api_base.internal_domain_resolver');
// We can't set URI's 'external' property to FALSE, because it will
// break the URL validation.
if ($resolver->isExternal($url)) {
$element['#attributes']['data-is-external'] = 'true';
if ($scheme = $resolver->getProtocol($url)) {
$element['#attributes']['data-protocol'] = $scheme;
}
$useTemplate = TRUE;
}
}
if ($useTemplate) {
$element['#title'] = [
'#theme' => 'helfi_link',
'#url' => $element['#url'],
'#title' => $element['#title'],
];
$element['#title']['#attributes'] = $element['#attributes'] ?? [];
}
return parent::preRenderLink($element);
}and it seemed to cover all our use cases I can think of. |
dragos-dumi-ibm
force-pushed
the
PLATTA-4940-LinkProcessor-fix
branch
from
2c0addd to
26eff25
Compare
Codecov Report
@@ Coverage Diff @@
## main #119 +/- ##
============================================
+ Coverage 62.46% 64.29% +1.82%
- Complexity 441 468 +27
============================================
Files 64 65 +1
Lines 1564 1641 +77
============================================
+ Hits 977 1055 +78
+ Misses 587 586 -1
... and 1 file with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
@tuutti I've reverted to the initial version with the updates you've suggested |
|
@dragos-dumi-ibm Thanks! I think the |
|
Kudos, SonarCloud Quality Gate passed!
|
|
indeed, but just because currently any other icon than external is attached to a title that has markup. I removed it now. just to mention, with this markup check, the internal buttons are still sent to twig, although is not necesary... but thinking that can't be that many in one page, it probably has minimal impact. Thanks, |
Currently all type link elements gets the title pass trough a twig.
This causes unnecessary load time, especially if toolbar is present, or a page with lots of links.
One title link rendering takes 0.8- 1.4ms as profiled, and some pages may have 150link + toolbar 300-400 links
I've taken a look into hdbt link twig and replicated the conditions where there's more things printed out besides the title -
Only in this case it makes sense to actually overwrite the title with a twig.
https://github.com/City-of-Helsinki/drupal-hdbt/blob/main/templates/module/helfi_api_base/helfi-link.html.twig
After we merge this, I'll open a PR into hdbt theme with the a comment in helfi-link.html.twig to warn as conditions in this link processor needs to be updated if conditions in twig changes.