Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: ability to add alt text to images #421

Open
wants to merge 21 commits into
base: main
Choose a base branch
from

Conversation

Sembauke
Copy link
Member

@Sembauke Sembauke commented Jan 31, 2024

When adding a new image:
image

When editing a new image:
image

Closes #392

@Sembauke
Copy link
Member Author

Sembauke commented Feb 1, 2024

So currently there are a view problems:

  1. There is no good way of transporting variables to the new custom image extension. This is because CustomImage is extended from Image (TipTap) which holds a schema for all possible attributes that may be present on the Node.
  2. Updating caption/alt text in the DOM is not the same as updating the image in the local Strapi instance. Meaning when updating the image alt text, it should be updated in the DOM and database. After that, the content of the editor needs to be saved. Otherwise the image alt or caption does not align with the one in the database.

CC: @sidemt and @ojeytonwilliams

@sidemt
Copy link
Member

sidemt commented Feb 1, 2024

@Sembauke Thank you for catching this, I didn't know Strapi saves alternativeText with image files.

I think we can ignore the value in alternativeText attribute, and only save/update alt text in the DOM (HTML).

Ghost doesn't have a separate attribute for alt text either so it shouldn't be a problem (it has feature_image_alt on the latest version, but not on v3, the version we are using.)

The modal to add/edit image is looking awesome 👍

@Sembauke Sembauke marked this pull request as ready for review February 2, 2024 08:48
@Sembauke Sembauke requested a review from a team as a code owner February 2, 2024 08:48
@Sembauke
Copy link
Member Author

Sembauke commented Feb 5, 2024

One more thing that need to be solved :
At the moment it is possible to click the "Save" button multiple time when adding a new image. This adds the image multiple times into the editor. This is because the image is still being uploaded before the "Upload image modal" is closed.

There is one other issue in the editor but I will have to do more research before I can comment on that.

I have removed the logic that uploads the alternative text and captions to the database. Hopefully we will not need to re-add this in the future. This makes things a lot easier. There won't be any need to auto save as well!

CC: @sidemt

@sidemt
Copy link
Member

sidemt commented Feb 6, 2024

@Sembauke

I have removed the logic that uploads the alternative text and captions to the database.

I believe this is OK :)

At the moment it is possible to click the "Save" button multiple time when adding a new image. This adds the image multiple times into the editor. This is because the image is still being uploaded before the "Upload image modal" is closed.

Is it possible to disable the save button until the upload completes? If it's complicated, we can leave it to another PR.

Copy link
Member

@sidemt sidemt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, it's looking good!

Currently, the preview doesn't seem to render <figure> and <figcaption> elements even if the image has a caption. It is rendered properly in the editor. Could you make it appear in the preview too?

Edit: Oliver's PR #427 may fix this issue.

@Sembauke
Copy link
Member Author

Sembauke commented Feb 7, 2024

Hey @sidemt,

Is it possible to disable the save button until the upload completes? If it's complicated, we can leave it to another PR.
This should be resolved now.

I will wait on #427 to go in.

@Sembauke Sembauke requested a review from sidemt February 7, 2024 09:52
sidemt
sidemt previously approved these changes Feb 10, 2024
Copy link
Member

@sidemt sidemt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍 Sorry for keeping you waiting!

@sidemt
Copy link
Member

sidemt commented Feb 10, 2024

@Sembauke Sorry, do you mind resolving the conflicts again? I tried doing it myself, but my attempt seems to erase some code in the main branch (the line using useColorMode()) and results in a lint error. I'm not sure what I'm doing wrong.

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.

Captions and alt text for images
2 participants