-
Notifications
You must be signed in to change notification settings - Fork 16
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] wasm build & gitignore #3
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.
Thank you very much. Here are some comments about the PR:
CMakeLists.txt
Outdated
@@ -31,4 +38,32 @@ target_link_libraries(ftxui-starter | |||
PRIVATE ftxui::component # Not needed for this example. | |||
) | |||
|
|||
install(TARGETS ftxui-starter RUNTIME DESTINATION "bin") | |||
add_custom_command(TARGET ${PROJECT_NAME} |
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 think you only have to copy the "index.html" and the python script into the build directory.
Could you use:
https://github.com/ArthurSonzogni/termBreaker/blob/6c882381ca41f2cce693e3cb39284d6c281ed5b4/CMakeLists.txt#L216-L220
instead of all those lines? I worry they are going to afraid newcomers ;-)
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 understand your worries, I personally feel that a newcomer would enjoy to have a clear list of files he need to keep and files he can dump (build files) having a target file (or any other name) makes it easier to identify mandatory files for its project to work (at least for wasm part)
waiting for your answer to remove or not this part
README.md
Outdated
cd build_emscripten | ||
emcmake cmake .. | ||
make -j3 | ||
cd ../target |
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.
This line isn't useful anymore, because of my previous comment about not copying files into the "./target" directory.
src/main.cpp
Outdated
std::cout << screen.ToString() << std::endl; | ||
|
||
return EXIT_SUCCESS; | ||
#include <memory> // for allocator, __shared_ptr_access |
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.
This change seems unrelated to your patch. Could you revert it?
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 had to include this change since the previous example wasn't working for an unknown reason in wasm. I suppose this is due to the fact that we aren't in a loop and thus it hasn't time to process the render before it exits.
Maybe this is just an issue on my machine, if you could try it yourself and let me know i would appreciate
PR:#3 author: Guerout-Arnaud
Thanks for your pull request! |
PR:ArthurSonzogni/ftxui-starter#3 author: Guerout-Arnaud
PR:ArthurSonzogni/ftxui-starter#3 author: Guerout-Arnaud
PR:ArthurSonzogni/ftxui-starter#3 author: Guerout-Arnaud
This PR is here to answer and close this issue