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

Add link to internal package #1095

Merged
merged 1 commit into from Jan 10, 2019

Conversation

Projects
None yet
6 participants
@vinkla
Copy link
Contributor

vinkla commented Dec 3, 2018

Description of the Change

This pull request add link to the internal packages in the atom/atom repository from the detail package view.

Alternate Designs

N/A

Benefits

If the user clicks on the repository link in the about package for example, it will be taken to the following link:

https://github.com/atom/atom/

With this update it will instead be taken to the following link:

https://github.com/atom/atom/tree/master/packages/about

Possible Drawbacks

N/A

Applicable Issues

N/A

@rsese

This comment has been minimized.

Copy link
Member

rsese commented Dec 4, 2018

Thanks @vinkla! Can you add tests for this?

@rsese rsese added the needs-testing label Dec 4, 2018

@vinkla

This comment has been minimized.

Copy link
Contributor Author

vinkla commented Dec 4, 2018

@rsese sure! Actually tried to find the test for the previous behaviour but couldn't find it. Do you have any example of similar tests? Maybe in another package?

@maralv

This comment has been minimized.

Copy link
Contributor

maralv commented Dec 5, 2018

@rsese sure! Actually tried to find the test for the previous behaviour but couldn't find it. Do you have any example of similar tests? Maybe in another package?

Eventhough not an accepted PR yet, you might find value in taking a look at my tests here:
eef8d84

Basically, I use a Jasmine spy to monitor shell.openExternal and check the argument for the correct URL. ✔️

Show resolved Hide resolved lib/package-detail-view.js Outdated
Show resolved Hide resolved lib/package-detail-view.js Outdated
@vinkla

This comment has been minimized.

Copy link
Contributor Author

vinkla commented Dec 5, 2018

I've added tests based on @maralv suggestion. It seems the AppVeyor fails but it doesn't seem to be related to this pull request.

@rsese

This comment has been minimized.

Copy link
Member

rsese commented Dec 5, 2018

Thanks @vinkla and thanks to everyone who chimed in to help 🙇 Someone from the team will take a look as soon as they can!

seems the AppVeyor fails but it doesn't seem to be related to this pull request.

👍

@rsese rsese removed the needs-testing label Dec 5, 2018

@daviwil
Copy link
Member

daviwil left a comment

Thanks a lot for this PR @vinkla!

@daviwil daviwil merged commit 01f1f2e into atom:master Jan 10, 2019

1 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.