-
Notifications
You must be signed in to change notification settings - Fork 176
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
Add option to hide alt text #116
base: master
Are you sure you want to change the base?
Conversation
This is a great addition. This is particularly useful for a multi language app like mine where the alt text display text in languages that are not that of the user. |
@mtibben really sorry for the disturbance, but any idea on when can this get a review 😀, Would really appreciate it, if you can give it a look. |
@mtibben friendly ping. Would you be okay with this change? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally I do not agree with the direction of this change, but given it's behind a feature flag I think it's probably fine.
src/Html2Text.php
Outdated
* show/hide alt text for images | ||
*/ | ||
|
||
protected function convertImages(){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Poor method naming. This does not actually convert images.
I'd suggest the same approach as convertBlockquotes
and covnertPre
where they take the value and actually perform the conversion. This will need to be done immediately after the current preg_replace
to ensure b/c.
a8cb26a
to
81999a8
Compare
If you have this problem, you can replace it first while waiting for an updated version: $content = str_replace('alt=', 'alta=', $content);
$content = str_replace('href=', 'hrefa=', $content);
$html = new Html2Text($content );
$html->getText(); |
we switched to a forked version https://github.com/hamza221/html2text in the meantime, but it would be great to switch back to the original package |
@andrewnicols Please review the code. Thanks |
adding the option of showing/hiding alt text from images