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

FIX: removed double link when custom_display_url (#5400) #5544

Merged
merged 1 commit into from
Jun 24, 2020

Conversation

LaurMerl
Copy link

Hi @kevin-bates, @liamhuber, I had a look at the issue in question and have a suggestion on how to fix it.

This is my first contribution so please do let me know if anything about this PR isn't in line with how things are done - and apologies in advance if so!
Thanks in advance!

Copy link
Member

@kevin-bates kevin-bates left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @LaurMerl - thank you for your contribution and welcome to Jupyter!

Your change works nicely. I would recommend though that this messaging also reflect that only one URL will be offered:

    Or copy and paste one of these URLs:
        http://foo/bar/?token=e2cc24936010a7557df551946fb6852952dfe17030987f19

and make this clause conditional as well.

@LaurMerl
Copy link
Author

Hey @kevin-bates, good catch, completely overlooked that!
That also has been taken care of, please let me know what you think of it.

Copy link
Member

@kevin-bates kevin-bates left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Laura - this looks good.

@kevin-bates kevin-bates merged commit 2a21e2a into jupyter:master Jun 24, 2020
@liamhuber
Copy link

@LaurMerl Thanks!!!

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 23, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants