-
-
Notifications
You must be signed in to change notification settings - Fork 5.7k
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
Fixing behaviour for when an link is root-relative #850 #877 #1339
base: develop
Are you sure you want to change the base?
Conversation
When rendering image tags, would render the image URL from base path if the image URL is root-relative, ie starts with '/'. This is would make the behaviour of image URLs more predictable.
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/docsify-core/docsify-preview/kr6q5t7my |
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit eee7754:
|
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.
Can you add some test case here ?
also, it might be a breaking change ! may be doing this behind an option?
cc @docsifyjs/core thoughts?
Agreed on doing this behind an option, will work on that and some tests. Thanks! |
If rootRelativeImageURL is false (default), current behaviour remains the same. If rootRelativeImageURL is true, images with root-relative URLs would be rendered exactly as specified. E.g. ![](/assets/image.png) --> <img src="/assets/image.png" /> If rootRelativeImageURL is a string, that would be treated as the path to which root-relative image paths resolve. E.g.: config: { rootRelativeImageURL: "docs" } ![](/assets/image.png) --> <img src="/docs/assets/image.png" />
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.
This is good. Yeah, it's a breaking change, so until V5 it will need to be behind an option.
We are looking to add tests to all new changes. See docsify.test.js
for an example of a test that loads a local Docsify site. Then you can write some code that checks that the image tag has the right src.
Added new behaviour and tests. Config for this follows the behaviour for some of the existing options like
|
Co-authored-by: Anix <anik220798@gmail.com>
Thanks |
Please don't merge yet, will update docs as well. |
This broke the case where config.rootRelativeImageURL is set to true --> empty string --> false, which is treated differently by the compiler. Added descriptin in configuration.md
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.
thx for ur input. 👍
cc @trusktr can you review this ? you have a request change pending I suppose. |
I hope this bug can be solved soon... |
still not fixed |
Summary
This PR addresses #850 and most likely #877
When rendering image tags, render the image URL starting from the base path if the image URL is root-relative, e.g.
/assets/my-image.png
. Currently, root-relative paths are treated in the same way as normal relative paths which is not consistent with the behaviour of HTML img tags or markedjs compilation output.Example
Given basePath = "" and the current file being
/advanced/guide.md
:![](/assets/my-image.png)
previously compiled to<img src="/advanced/assets/my-image.png></img>
, where it is generally expected to render<img src="/assets/my-image.png></img>
This is in line with how HTML
img
tags work and how marked.js compiles its image tags.More advanced example
Given basePath = "/docs/" and directory structure:
In docs/subfolder/guide.md:
While previous behaviour would have been:
What kind of change does this PR introduce? (check at least one)
If changing the UI of default theme, please provide the before/after screenshot:
Does this PR introduce a breaking change? (check one)
If yes, please describe the impact and migration path for existing applications:
This can possibly break existing repos who are relying on normal relative path behaviour even when using root-relative paths in their markdown, and hence have images located within subfolders. I have looked through some repos however and could not find any instances of this, since this is generally not how image link URLs are expected to behave.
The PR fulfills these requirements:
fix #xxx[,#xxx]
, where "xxx" is the issue number)You have tested in the following browsers: (Providing a detailed version will be better.)
If adding a new feature, the PR's description includes:
To avoid wasting your time, it's best to open a feature request issue first and wait for approval before working on it.