Fix markdown image src transformation in some usages
#1375
+95
−53
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The problem
In the
terminalpackage’s README, I've got a screenshot to demonstrate the functionality. I was inclined to do what other builtin package READMEs have done and use GitHub as an image host. But I think a lot of those tricks no longer work! Try as I might, I can't get an old-stylecloud.github.com/assetsURL when I upload an attachment to a GitHub issue; they're all URLs that strongly imply that they are not permanent and may expire at some point in the future. (Probably to cut down on people using GitHub as an image host.)The next best thing is to check the screenshot into source control and reference it from the README as a relative URL; this works fine on GitHub itself. But the image was showing up as broken on the
terminalpackage’s settings page (we show the package's README after all the settings, keybindings, and whatnot).I dug into why this was happening and it was mostly some spaghetti in a section of
atom.ui.markdown.renderthat doesn't have any test coverage. I have taken the cowardly path and failed to add to that test coverage, but I do think this code works better than it did before.The fix
When you call
atom.ui.markdown.render, you can specify arootDomain(if your Markdown came from a remote URL) or afilePath(if your Markdown came from a file on disk). That function has a section that handles the possible need to transform imagesrcvalues (relativesrcvalues, etc.), but that section was being skipped entirely whenrootDomainwas omitted. This meant thatsettings-view’sPackageReadmeViewdidn't get the benefit of imagesrctransformation at all.Once that was fixed, there was more to solve, including an accidental negation in a conditional, and a reference to a nonexistent variable.
We also did something a bit strange: if
rootDomainwas specified at all, we tried a way of resolving the image that would only work ifrootDomainreferred to a GitHub URL. Instead, I made it so that that tactic was only attempted if we actually sawgithub.inrootDomain; otherwise I treated it like an arbitrary URL and combinedrootDomainandsrcto produce an absolute URL.Testing
This PR is against
master, but I don't have a great way of testing this until the Electron 30 bump lands — because my test package isterminal, which isn't written to be compatible with Electron 12.I'll see if I can update one of my other packages so that the README references a screenshot that lives in the repo. Then someone can install it via GitHub and verify that way.