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

Relative image URLs in package READMEs are now resolved correctly #996

Merged
merged 5 commits into from Oct 26, 2017

Conversation

Projects
None yet
6 participants
@ftm
Contributor

ftm commented Sep 14, 2017

Description of the Change

When rendering package READMEs, whilst sanitizing the source image, URLs are now checked to see if they are absolute or relative and if they are relative then they are resolved appropriately. For example for non-installed plugins the base URL would be the GitHub repo however if they are installed it uses the path of the installed plugin.

Before:
relative url before

After:
relative url after

Alternate Designs

I considered attempting to match up each image in the Markdown with its counterpart on its Atom.io page or its README on GitHub and using those URLs. I chose this solution in the end as it was much simpler and in the cases I've tested still seems to work.

Benefits

Packages using relative URLs for images in their README files as recommended by GitHub will now be rendered correctly

Possible Drawbacks

Any URL scheme not recognized by the code I have written will be assumed to be a relative path so the image URL may be altered incorrectly.

Applicable Issues

The applicable issue would be here

Show outdated Hide outdated lib/package-readme-view.js Outdated
Show outdated Hide outdated lib/package-readme-view.js Outdated
Show outdated Hide outdated lib/package-readme-view.js Outdated
Show outdated Hide outdated lib/package-detail-view.js Outdated
Show outdated Hide outdated lib/package-detail-view.js Outdated
Show outdated Hide outdated lib/package-detail-view.js Outdated

@50Wliu 50Wliu added the needs-review label Sep 15, 2017

@iolsen

This comment has been minimized.

Show comment
Hide comment
@iolsen

iolsen Sep 18, 2017

We'd love to see this change go in if you can address @50Wliu's feedback.

iolsen commented Sep 18, 2017

We'd love to see this change go in if you can address @50Wliu's feedback.

@ftm

This comment has been minimized.

Show comment
Hide comment
@ftm

ftm Sep 18, 2017

Contributor

I've addressed most of it, I'm just waiting for some clarification on a couple of points

Contributor

ftm commented Sep 18, 2017

I've addressed most of it, I'm just waiting for some clarification on a couple of points

@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu

50Wliu Sep 18, 2017

Member

Sorry, I'll try to get back to you soon!

Member

50Wliu commented Sep 18, 2017

Sorry, I'll try to get back to you soon!

@ftm

This comment has been minimized.

Show comment
Hide comment
@ftm

ftm Sep 18, 2017

Contributor

Cool, no rush

Contributor

ftm commented Sep 18, 2017

Cool, no rush

@damieng

This comment has been minimized.

Show comment
Hide comment
@damieng

damieng Oct 10, 2017

Contributor

I'd love to get this merged in - there seems to be some conflicting files now though - possibly because of my change from our own sanitization to dompurify.

Contributor

damieng commented Oct 10, 2017

I'd love to get this merged in - there seems to be some conflicting files now though - possibly because of my change from our own sanitization to dompurify.

@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Oct 25, 2017

Contributor

I'm going to attempt to resolve these conflicts. 🤞

Contributor

nathansobo commented Oct 25, 2017

I'm going to attempt to resolve these conflicts. 🤞

@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu

50Wliu Oct 26, 2017

Member

@nathansobo Just so that you're aware of my last review comment that we're not sure how to resolve:
the readme location for packages that aren't installed (i.e. you're browsing in the Install view) is currently hardcoded to readmeSrc = repoUrl + '/blob/master/' instead of the last-published release tag.

Member

50Wliu commented Oct 26, 2017

@nathansobo Just so that you're aware of my last review comment that we're not sure how to resolve:
the readme location for packages that aren't installed (i.e. you're browsing in the Install view) is currently hardcoded to readmeSrc = repoUrl + '/blob/master/' instead of the last-published release tag.

@nathansobo nathansobo merged commit e962a54 into atom:master Oct 26, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Oct 26, 2017

Contributor

Thanks for contributing this @frasertmay!

Contributor

nathansobo commented Oct 26, 2017

Thanks for contributing this @frasertmay!

@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Oct 26, 2017

Contributor

@50Wliu Hey, sorry, I missed your comment somehow before merging this 😬 . Guess the PR didn't auto-update? Should I roll this back? Still seems like an overall improvement to have some sort of README images though it won't be 100% due to the fact they could be deleted, master might not be there, etc.

Contributor

nathansobo commented Oct 26, 2017

@50Wliu Hey, sorry, I missed your comment somehow before merging this 😬 . Guess the PR didn't auto-update? Should I roll this back? Still seems like an overall improvement to have some sort of README images though it won't be 100% due to the fact they could be deleted, master might not be there, etc.

@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu

50Wliu Oct 26, 2017

Member

Yeah, it's definitely an improvement. I don't think it needs to be reverted; I'll create a follow-up issue for it.

Member

50Wliu commented Oct 26, 2017

Yeah, it's definitely an improvement. I don't think it needs to be reverted; I'll create a follow-up issue for it.

@ftm

This comment has been minimized.

Show comment
Hide comment
@ftm

ftm Oct 26, 2017

Contributor

Thank's for sorting out the merge conflicts @nathansobo!

@50Wliu, if you're happy to sort out the URL issue in a separate issue/PR I'll delete the branch.

Contributor

ftm commented Oct 26, 2017

Thank's for sorting out the merge conflicts @nathansobo!

@50Wliu, if you're happy to sort out the URL issue in a separate issue/PR I'll delete the branch.

@Ben3eeE

This comment has been minimized.

Show comment
Hide comment
@Ben3eeE

Ben3eeE Oct 26, 2017

Member

🎉

Member

Ben3eeE commented Oct 26, 2017

🎉

@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu

50Wliu Oct 26, 2017

Member

@frasertmay Feel free to delete the branch.

Member

50Wliu commented Oct 26, 2017

@frasertmay Feel free to delete the branch.

@50Wliu 50Wliu referenced this pull request Oct 26, 2017

Open

READMEs should fall back to latest tag, not master #1012

1 of 1 task complete

@ftm ftm deleted the ftm:readme-relative-urls branch Oct 26, 2017

@nathansobo nathansobo self-assigned this Oct 27, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment