FEATURE: Allow users to edit alt text from the image preview in the editor (PR #14480)

potato quality edit-alt-text gif showing how to edit the text

Related discourse thread

We want to give users who are not familiar with the markdown syntax the ability to edit the alt text in the image preview.

This implementation is V0, and we might consider forcing users to enter a sufficiently lengthy description in the future.

GitHub

I’m not sure if there’s a better way to whitelist these. I tried adding these in another file’s helper.allowList and it seems it’s universal. I’d feel better off if I could do something more precise like input[class=alt-text-input][hidden=true].

Also renamed the file to a more generic image-controls.js, since I don’t think it’s possible to have multiple image renderer rules?

Actually caught a sneaky bug with the multiple image test case. Putting this test case below the ‘single image’ one since the query selectors are slightly annoying.

I had the option to

  • put all elements here and hide/show them accordingly (which is what I’m doing here),
  • or dynamically add/remove elements in composer-editor

I chose the former (this) as it seemed to be lower in complexity.

Also, I looked around for how aria-labels are applied but don’t seem to find a lot of them. I’ve also hard coded the value but it seems like theres a couple of occurrences of translated aria labels. Would be nice to get some input here.

I don’t see where this is used?

Maybe we should check for what happens when we delete alt text ?

You could create some setup function, and use it in two tests

I’m not sure I understand your question, but you could make it dynamic using I18n.t and concerning the content of the key I think I would go with something descriptive: “alt text used for the above image” , what do you think?

I think what you did here is good. Just a note we try to only use the term allowList now.

I see its lacking spaces here, be sure to configure your editor with prettier/eslint to follow our code standards please

if possible I prefer to avoid direct styling on html elements

I’m not sure how much time yet you have to allocate on this, but it would be a good exercice to remove jquery usage of this function. We should do it with scaling too but that’s outside the scope of this PR.

It should be rather simpler to do.

One harder thing to do would be to think about the listeners here, I’m not a fan of this off/on pattern, maybe try to see if you can think of a place to have a better hook to cleanly removeEventListener. No worries if you can’t find a way, I know this is how we currently do in core.

I understand it’s a V0 and this is good enough UX wise, but I think the input kinda feels clunky as it’s larger than the image, something doesn’t feel right. Not sure if we can think of something better.

Yeah. Thanks for spotting this! It shouldn’t be here and was only added to debug the tests.

Overall I love the way this looks, there is a bit of odd spacing between 75% and 50%, any idea what is going on there?

I get @jjaffeux concern with smaller images, an alternative we can explore is moving to 2 lines when we have a narrow image, eg:

100% 75% 50%
image (pencil)

That should remove a bit of the awkward and be at least as good as it is today given 100,75,50 takes up so much space anyway.

Quality of the work is great, I love that it is well tested and well thought out!

we try to only use the term allowList now

Do you mean allowList is temporary? Can you elaborate please?

shakes fist I trusted you webstorm!

Ahh sorry, a year or 2 ago we made a policy that in all places where we had abcWhitelist and abcBlacklist instead we use abcAllowList and abcDenyList, slightly less loaded terms.