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

Fix/Issue-863: http check on toc image #874

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

rrolonNelson
Copy link

@rrolonNelson rrolonNelson commented Oct 27, 2023

Thank you for contributing to Quire! Please complete the form below to submit your pull request for review.

For the Title of this pull request, please use the format "Type/Issue-#: Brief description." For Type, the options are Fix, Feature, Docs, or Chore. Issue-# is only needed if this pull request addresses an exisiting issue.

Checklist

Please put an X within the brackets that apply [X].

  • I have read the CONTRIBUTING.md file

  • I have made my changes in a new branch and not directly in the main branch

  • This pull request is ready for final review by the Quire team

Is this pull request related to an open issue? If so, what is the issue number?

#863

Please briefly describe the goal of this pull request and how it may impact Quire's functionality.

This imitates the HTTP check in image-tag.js so that way we can pull an image src that has a full URL without it turning into a relative URL

Please describe the changes you made, and call out any details you think are particularly relevant for the Quire team to note in their review.

Does this pull request necessitate changes to Quire's documentation?

no

Include screenshots of before/after if applicable.

Additional Comments

I tried to get the alt text to be brought in properly as well but it doesn't seem to be working.

mphstudios
mphstudios previously approved these changes Nov 15, 2023
Copy link
Member

@mphstudios mphstudios left a comment

Choose a reason for hiding this comment

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

@rrolonNelson thank you for submitting this pull-request! These changes help make the handling of the image src more consistent and nicely follow the JSDoc conventions. To fix the image alt text for TOC items I submitted #878.

@mphstudios mphstudios dismissed their stale review November 29, 2023 19:52

Current solution introduces bug for epub output

@mphstudios
Copy link
Member

mphstudios commented Nov 29, 2023

@rrolonNelson reviewing this solution locally we discovered that the change will break images for the epub output; until we can revisit the solution, if you do not need epub output, the change in this pull-request can be made directly to a local Quire publication.

@rrolonNelson
Copy link
Author

change will break images for the epub output;

@rrolonNelson reviewing this solution locally we discovered that the change will break images for the epub output; until we can revisit the solution, if you do not need epub output, the change in this pull-request can be made directly to a local Quire publication.

Good catch. We are not currently using the epub. I will take another look and see if I can resolve the issue for the epub and update this PR.

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.

2 participants