-
-
Notifications
You must be signed in to change notification settings - Fork 26
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 to ignore hash in images #60
Conversation
3b1d2e5
to
2089092
Compare
Codecov Report
@@ Coverage Diff @@
## main #60 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 12 12
Lines 322 325 +3
=========================================
+ Hits 322 325 +3
Continue to review full report at Codecov.
|
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.
Awesome! Thanks for working on a PR!
From the changed test, and the missing handling of full URLs, I think it’d be good to apply some changed though. Annotated inline!
lib/find/find.js
Outdated
if ( | ||
// Ignore "headings" in image links: `image.png#bordered` | ||
if (numberSignIndex !== -1 && nodeType === 'image') { | ||
value = value.slice(0, numberSignIndex) | ||
} else if ( |
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.
I think this is interfering with query params, as can be seen by the fixture change.
This is also not handling the case for full URLs: the if (url)
before this.
Can this be reverted and added somewhere else?
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.
L255 can be solved with:
return normalize(path.resolve(config.root, value + (nodeType === 'image' ? '' : url.hash)), config)
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.
Then after the old L239, now L242, add a new:
numberSignIndex = value.indexOf(numberSign)
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.
The you can add your if-statement on old L241, new L244:
if (numberSignIndex !== -1 && nodeType === 'image') {
value = value.slice(0, numberSignIndex)
}
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.
L255 can be solved with:
return normalize(path.resolve(config.root, value + (nodeType === 'image' ? '' : url.hash)), config)
As I suppose - you mean line 225 :)
It makes code complexity at 21, but the limit is 20 - can you help me with refactor?
@@ -1,6 +1,6 @@ | |||
# Query params | |||
|
|||
Link to relative heading ![link](?foo=bar#query-params) | |||
Link to relative heading [link](?foo=bar#query-params) |
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.
Could this be reverted, unless I’m missing something?
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.
Ah, I missed that in your PR body.
because it is a "Link" not "image", so this looks like a mistake (and prevented tests from working with my code).
It sure looks that way. But, as an image is expected by browsers, they will send something along the lines of an expected image/png, image/jpg, ...
mimetype, for which the server could respond with an image.
I think it’s good to keep it in!
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.
Sure, so wouldn't it be better if we create a new case for image and fix the current for heading?
Link to relative heading [link](?foo=bar#query-params) // change to be valid
Link to an ![image](./examples/image.jpg?foo=bar) // current image test without hash
Link to an ![image](./examples/image.jpg?foo=bar#client-params) with hash // new image test with hash
to keep test clean and understandable?
I have pushed all the proposed changes, but there is complexity warning, so we need refactor. |
Thanks for your patience! Released! |
Thank you very much! |
Hi!
The puprose of this feature is to fulfill following requirement:
When linting image paths, hashes should be ignored just like question signs.
AFAIK text after
#
sign in image sources are never used as a part of target but rather as an additinal data for resulting image. For example: https://www.xaprb.com/blog/how-to-style-images-with-markdown/ describes how to style images using content after#
sign in image source. There is also use case in: https://www.gatsbyjs.com/plugins/gatsby-remark-image-attributes/ where#content
is converted toimg
attributes in resulting HTML.For example, currently if
image.png
file exists but we lint:There will be an error:
Despite the fact, that the image exists and it will be rendered properly.
Other changes
In this PR I also changed
test/fixtures/query-params.md
to
because it is a "Link" not "image", so this looks like a mistake (and prevented tests from working with my code).