- Issue created by @sascha_meissner
- Status changed to Needs review
11 months ago 2:31pm 10 August 2023 - ๐ฉ๐ชGermany sascha_meissner Planet earth
I also added the functionality to drupalImage, for me everything is working pretty good, please review my changes in the 3380223-mediainsert---make branch :-)
Whats still open is that the allowed_html changes by adding the title attribute, iยดm not sure how to update this and need help whats the best way to do so. Also, as this functionality is missing in ckeditor5 library it might make sense to port it there.
- Merge request !4590Issue #3380223: MediaInsert - Make image title attribute configurable โ (Open) created by smustgrave
- Open on Drupal.org โEnvironment: PHP 8.2 & MySQL 8last update
11 months ago Not currently mergeable. - Status changed to Needs work
11 months ago 7:25pm 14 August 2023 - ๐บ๐ธUnited States smustgrave
Opened the MR so it may reviewed.
Have not reviewed it myself but something like this I can see will need test coverage.
Thanks
- ๐ฉ๐ชGermany sascha_meissner Planet earth
Hey @smustgrave, thanks for your reply. Surely i will update/adjust the tests for media to also check for the functionality, but iยดd say before that the actual code change should be reviewed and in this process probably further developed. Itยดs the first time I try to contribute something to core, especially using MRยดs so please go easy on me :) I actually dont know why the branch is currently not mergeable, i dont know where i find any output about it. But yes iยดd love to write the tests as soon as the feature is finalized.
- ๐ฉ๐ชGermany sascha_meissner Planet earth
One thought i have is if there should be a default text for the title attribute, maybe it makes sense to use the text-alternative? Ideally this would be configurable, on most of our systems, in example, medias have a title field with a designated title additionally to the text-alternative. This could possibly be a setting within the media tab in text-format edit page?
- ๐ฉ๐ชGermany FeyP
@sascha_meissner Thanks for working on this. The merge is blocked because there are new commits on 10.1.x and your branch has diverged, so a fast-forward merge is not possible. To fix this, you need to rebase the MR branch. You can see this when you open the MR page in GitLab:
> Merge blocked: fast-forward merge is not possible. To merge this request, first rebase locally.
See here for more info: https://www.drupal.org/docs/develop/git/using-gitlab-to-contribute-to-dr... โ
To get an early review or a second opinion for design decisions, you could ask in #ckeditor5 or #contribute in Drupal Slack.
- ๐ฉ๐ชGermany sascha_meissner Planet earth
Itยดs been some time ... just rebased origin/10.1.x into my issue fork (which was a little nasty because i needed to rebuild ckeditor5 for every commit :P ) Also didnt manage yet to join the slack or do any other progress related to the topic, at least you can patch 10.1.8 now with the diff
- ๐ง๐ชBelgium Wim Leers Ghent ๐ง๐ช๐ช๐บ
Epic work here! ๐
- This is modifying
DrupalInsertImage
too? Would this title be more accurate? Can you update the issue summary to explain why we should modify both simultaneously in this issue? - Also: I recall that the
title
attribute can be problematic wrt its impact on accessibility. Can you also explain in the issue summary why this would not be a concern? - Finally: I see no test additions nor changes ๐ There is a lot of test coverage for related functionality, expanding test coverage should be relatively straightforward.
- This is modifying