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: [Emscripten] open links in browser #8655
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Completely forgot that we support opening links :D
96266f4
to
0a774e9
Compare
Regarding commit message: Next we have Next we have |
0a774e9
to
c4b02a3
Compare
@TrueBrain Should be ready for (re)review now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most excellent :) Some minor coding-style bla, but otherwise looks really good.
The comment is really well written, thank you for that :)
c4b02a3
to
529ad43
Compare
529ad43
to
69e2fc4
Compare
@TrueBrain Sorry for the coding style mistakes; I haven't had a chance to read through the guide yet and configure my editor. I should probably do that. 😉 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@TrueBrain Sorry for the coding style mistakes; I haven't had a chance to read through the guide yet and configure my editor. I should probably do that. 😉
No worries; I always feel a bit like a nag to complain about it, but I also know it is the fastest way to learn any coding style a project has: by being corrected :D So I hope you don't mind, one more I missed first time around, and in your latest fix something went a bit wonky :P
Tnx a lot for this :D
src/os/unix/unix.cpp
Outdated
@@ -28,6 +28,10 @@ | |||
#include <SDL.h> | |||
#endif | |||
|
|||
#ifdef __EMSCRIPTEN__ | |||
#include <emscripten.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I totally missed this, but otherwise my fellow devs will hurt me: please add a tab between #
and include
, as seen in the blob just below this.
Currently not all code is converted yet, but new code should fix this :)
if (leftButtonDown) { | ||
document.addEventListener("mouseup", openWindow); | ||
} else | ||
openWindow(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You forgot the else
case :)
Also, please checkout out your spaces here, it seems to vary a lot :P Seems this file has 4 spaces for indentation, so it would be good to apply this to all new code here. But mostly, the if
has a different indentation than the else
:D
Edit: owh, it mixes tabs and spaces. Seems this js
file was done with spaces .. which is my bad :P But my editor does that by default .. better to stick with that for now :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The weirdness is probably VSCode choking on the mixed indentation style between this and other files; I'll hand-edit it to be consistent.
69e2fc4
to
1a97edc
Compare
Hopefully the last round this time. (Thank you for the corrections by the way.) |
os/emscripten/pre.js
Outdated
document.addEventListener("mousedown", e => { | ||
if (e.button == 0) { | ||
leftButtonDown = true; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my OCD helps me: this one is intended wrong :P :P
1a97edc
to
875e371
Compare
875e371
to
a4b11c6
Compare
Problem
The "Visit website" buttons were not functional on Emscripten since it doesn't support
xdg-open
.Description
This PR adds Emscripten-specific logic that opens the website using standard JS APIs.
Limitations
None that I can really think of.
Checklist for review
Some things are not automated, and forgotten often. This list is a reminder for the reviewers.