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

Make the links clickable in ban popup #520

Merged
merged 7 commits into from Oct 3, 2018
Merged

Make the links clickable in ban popup #520

merged 7 commits into from Oct 3, 2018

Conversation

ghost
Copy link

@ghost ghost commented Jul 29, 2018

No description provided.

@bls1999
Copy link
Collaborator

bls1999 commented Jul 30, 2018

Good pull! For future reference @BrokenBulb78:

  • Travis starts yelling when there's both logic and functions in the same file. The require link counts as logic, even though it's just calling another file.
  • Always use the complete URL with urlify (include the protocol, I.e. http/https/ftp)

Copy link
Collaborator

@bls1999 bls1999 left a comment

Choose a reason for hiding this comment

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

On second review, the function urlify isn't present in the http server functions. Therefore, this change would throw an error.

The reason urlify exists is 1) so that consecutive links in the multiplayer server don't make the code 5 million lines long and 2) to escape backticks very easily if need be. There's no need in the http server because there's barely any places where either of those things would be needed.

I would just write plain HTML for your link. Just make sure you use target="_blank" in your <a> HTML tag.

@ghost
Copy link
Author

ghost commented Jul 30, 2018

Fixed! Thanks...

And about the missing protocol, I noticed that when I made the pull so I immediately fixed it. 😂

@ghost ghost changed the title Urlify ban appeal link Make the links clickable in ban popup Jul 30, 2018
s
s
@jacob-grahn
Copy link
Owner

👍

@jacob-grahn jacob-grahn merged commit d570aa5 into jacob-grahn:master Oct 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants