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

Web binding #4

Merged
merged 29 commits into from
Feb 21, 2024
Merged

Web binding #4

merged 29 commits into from
Feb 21, 2024

Conversation

albho
Copy link
Contributor

@albho albho commented Feb 7, 2024

Will need to update with new .wasm files when ready and adjust initial wasm memory (orca.ts).

@albho albho requested a review from bejager February 7, 2024 20:49
Copy link

@mrrostam mrrostam left a comment

Choose a reason for hiding this comment

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

I'll go over orca.ts later

README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
binding/web/README.md Outdated Show resolved Hide resolved
binding/web/package.json Outdated Show resolved Hide resolved
binding/web/src/orca_worker.ts Show resolved Hide resolved
binding/web/src/orca_worker.ts Outdated Show resolved Hide resolved
demo/web/package.json Outdated Show resolved Hide resolved
@mrrostam mrrostam requested a review from ksyeo1010 February 7, 2024 21:38
@mrrostam
Copy link

Is it easy to add a 'download' feature to the demo?

binding/web/src/orca.ts Show resolved Hide resolved
@mrrostam
Copy link

Good job! once @ksyeo1010 and @bejager approve this, will merge it to master

Copy link
Contributor

@bejager bejager left a comment

Choose a reason for hiding this comment

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

Looks good! I played around with the demo and it works fine except when there is an exception.
E.g. when having a wrongly formatted custom pron: "A {|KH A S T OM } pron" the demo shows exception fine but it does not allow any changes to the text field anymore. One has to refresh the browser and init again.

demo/web/index.html Show resolved Hide resolved
Copy link
Contributor

@ksyeo1010 ksyeo1010 left a comment

Choose a reason for hiding this comment

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

Sorry I just saw this, the only thing I am wondering is why did we choose to go with callbacks instead of just returning Int16Arrays from the function itself after waiting for the async call? If anything, I think this should be similar to Leopard's methods.

binding/web/README.md Outdated Show resolved Hide resolved
binding/web/src/orca.ts Outdated Show resolved Hide resolved
binding/web/src/types.ts Outdated Show resolved Hide resolved
binding/web/src/orca.ts Outdated Show resolved Hide resolved
binding/web/src/orca_worker.ts Outdated Show resolved Hide resolved
@albho
Copy link
Contributor Author

albho commented Feb 21, 2024

I believe this is good to merge, the one failing link is to the demo which won't exist until this is merged

@mrrostam mrrostam merged commit 107d8b5 into main Feb 21, 2024
9 of 10 checks passed
@bejager bejager deleted the web-binding branch May 9, 2024 22:11
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.

4 participants