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

Palomar: Add alt text to ES Doc Text field #248

Conversation

jcsalterego
Copy link
Contributor

Adds alt text for images to the ES doc's text field.

For example:

Given

Text: i should buy a boat
Image 1 Alt: cat thinking i should buy a boat
Image 2 Alt: cat at the insurance office getting boat insurance

Then

ES Doc Text: i should buy a boat cat thinking i should buy a boat cat at the insurance office getting boat insurance

Copy link
Collaborator

@whyrusleeping whyrusleeping left a comment

Choose a reason for hiding this comment

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

Oh great idea, Thanks @jcsalterego !

@jcsalterego
Copy link
Contributor Author

I can't seem to reproduce the test failure locally. Is it a related failure?

@jcsalterego
Copy link
Contributor Author

@whyrusleeping
Copy link
Collaborator

Yeah, thats a known flaky test. I need to figure out how to deal with that better...

@whyrusleeping
Copy link
Collaborator

rerunning the tests to try and get a clean merge

@jcsalterego
Copy link
Contributor Author

Yeah, thats a known flaky test. I need to figure out how to deal with that better...

I figure adding a sleep between would remove the timing/race condition but I couldn't tell you why :)

@jcsalterego
Copy link
Contributor Author

Would love to know if there's anything I can do to help this get merged!

@jcsalterego
Copy link
Contributor Author

I assume this is on hold for a larger mapping change and task to reindex all posts?

@jcsalterego
Copy link
Contributor Author

I assume this is on hold for a larger mapping change and task to reindex all posts?

I think this would be good to get merged and online, even if it's only for a portion of the data (no backfill), and pending some larger changes in the schema.

@bnewbold
Copy link
Collaborator

This feature should be working in prod as of the past couple weeks, as part of a broader refactor of how we index posts. It took forever!

I'm going to close this PR; if the feature isn't working the way you expect, please re-open an issue. Sorry to leave you hanging on this helpful PR for so long, and thanks again for the contribution.

@bnewbold bnewbold closed this Dec 20, 2023
@jcsalterego
Copy link
Contributor Author

@bnewbold The big search refactor looks great and was worth the wait. Thanks!

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.

3 participants