Follow-up to #2152261: Clean up for tablesort-indicator.html.twig

Problem/Motivation

Fix the urls created for the image assets.

url() will produce /fr-ca/ or /drupal/ prefixed urls, I should have used file_create_url() for this but that doesn't exist in twig yet.

I've got a proposal started for that over at #2168231: Twig Functions needed in templates.
In the mean time I'd like to revert part of this so that it's working.
And the best solution for this template would be #2189729: Factor out tablesort-indicator.html.twig IMO.

Proposed resolution

Make the core template use the same pattern of building the asset file url in preprocess and pass it to the template.

Remaining tasks

User interface changes

API changes

CommentFileSizeAuthor
#2 interdiff.txt2.26 KBjoelpittet
#2 2310311-fix-tablesort-indicator-urls-2.patch2.12 KBjoelpittet
#1 2310311-1-sort-indicator-url-fix.patch2.06 KBjoelpittet
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented
Issue summary: View changes
Status: Active » Needs review
FileSize
2310311-1-sort-indicator-url-fix.patch2.06 KB

Here's the patch

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented
Issue summary: View changes
FileSize
2310311-fix-tablesort-indicator-urls-2.patch2.12 KB
interdiff.txt2.26 KB

Some doc fixes from @Cottser in #2073811: Add a url generator twig extension

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Quick fix

Looks great, thanks @joelpittet!

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

Seriously what is the point of this issue ... doesn't this just cause additional reroll work?
I would argue that this patch should add a file_create_url() twig extension instead.

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented

It's to fix my mistake and hopefully be a warning to others who use url() instead of file_create_url().

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

Yeah sure, let's get this in, unless the other one is faster.

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented

Dibs on re-roll for the url generator issue if this gets in first;)

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Needs work

#2073811: Add a url generator twig extension is in. Shall we make this issue about adding a file_create_url() twig extension instead?

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented
Status: Needs work » Closed (duplicate)

Debating using this as a file_create_url() because we have an issue discussing it over #2168231: Twig Functions needed in templates so for now closing it as a duplicate and may re-open and possibly consider calling that one a [META]

Thank you @alexpott

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented

I think we're covered by #2308187: Provide a twig extension for file_create_url so yes this can be closed as a dupe.