Skip to content

Conversation

@tonypaulbarker
Copy link
Contributor

@tonypaulbarker tonypaulbarker commented Oct 27, 2025

Closes #871

Continutes work to render the document files for the case where configuration provided in Drupal Core and known to be used on LGD sites prefers the description field to be used for the document link.

Requires code review.

Requires robust functional testing of the display in localgov_base of:

Document media configured to use name field
Document media configured to use description field
Document media with no name or description field
Drupal file upload fields
Webform file uploads

Intended logic

Use description if " use_description_as_link_text " is set in document media manage display and description field has value.

Otherwise

Use name field with fall backs as before

@rupertj
Copy link
Member

rupertj commented Oct 28, 2025

I've had a quick look at this - we don't need to get the description and set it as the title if the option's checked, as it's already been done by this point in the request.

If use_description_as_link_text is set, returning early and not doing anything else should be enough.

@tonypaulbarker
Copy link
Contributor Author

@rupertj thanks for having a look. use_description_as_link_text is probably true for almost every site since it has been in the default configuration, regardless of whether they have the description field or not to fill out. We do need to also see if the description is present. But maybe we can then return if it is.

@tonypaulbarker
Copy link
Contributor Author

Some weirdness in that we only modified localgov_base but CKEditor doesn't render the description for its label as expected when the field settings for the file description are active. The default media mode being a responsive image size doesn't make sense for files - should be 'default' instead.

@tonypaulbarker tonypaulbarker marked this pull request as ready for review November 3, 2025 14:46
@rupertj
Copy link
Member

rupertj commented Nov 4, 2025

I've tested this in H&F's site where the bug showed up and it's working well for me.

I've also exposed the media name field on it and disabled the option to use the file's description and it's showing the media name for media where I've set it.

There's a few spots where I think the code could be simpler and easier to follow, so I'll start a review.

In general though, I'd be happy for this to be merged as-is once the tests pass.

@finnlewis
Copy link
Member

I've tested this in H&F's site where the bug showed up and it's working well for me.

I've also exposed the media name field on it and disabled the option to use the file's description and it's showing the media name for media where I've set it.

There's a few spots where I think the code could be simpler and easier to follow, so I'll start a review.

In general though, I'd be happy for this to be merged as-is once the tests pass.

Based on Ru's comment, let's merge this and refactor code later if needed! Hope that's OK @rupertj :)

@finnlewis finnlewis merged commit 169a862 into 2.x Nov 4, 2025
25 of 29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Regression: Option to use the description for media links instead of the filename no longer works.

4 participants