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

NNotepad: UI update #249

Merged
merged 5 commits into from
Jun 18, 2024
Merged

NNotepad: UI update #249

merged 5 commits into from
Jun 18, 2024

Conversation

ibelem
Copy link
Contributor

@ibelem ibelem commented Jun 11, 2024

Updated UI with following changes:

  • Add navigation bar for logo, WebNN sample links, show generated code, show documentation and device type selector
  • Add code editor to replace the <textarea> which can show code lines
  • More style tuning

@inexorabletash @huningxin @Honry Please take a look and let me know if it's acceptable or not.

Screenshot 2024-06-12 094227

Copy link
Contributor

@huningxin huningxin left a comment

Choose a reason for hiding this comment

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

Does this change from "Nnodepad" to "NNotepad"? I cannot tell that from the diff.

@ibelem
Copy link
Contributor Author

ibelem commented Jun 12, 2024

@huningxin There was the typo in the svg logo, changed the text from "NNodepad" to "NNotepad" now, please refer to the updated new screen shot as attached.

@huningxin
Copy link
Contributor

The UI update looks good to me. I'll defer to @inexorabletash for decision. Thanks!

Copy link
Member

@inexorabletash inexorabletash left a comment

Choose a reason for hiding this comment

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

Nice work!

Some nitpicks, some requests.

Also, I'm seeing lots of console spam, e.g.:

  • Uncaught SyntaxError: Unexpected token <
  • Error: Unexpected usage at _EditorSimpleWorker.loadForeignModule

It's possible I've got things set up incorrectly locally, but I don't think those are coming from my code.

nnotepad/css/index.css Outdated Show resolved Hide resolved
nnotepad/css/index.css Outdated Show resolved Hide resolved
nnotepad/css/index.css Outdated Show resolved Hide resolved
nnotepad/js/index.js Outdated Show resolved Hide resolved
nnotepad/js/index.js Outdated Show resolved Hide resolved
@ibelem
Copy link
Contributor Author

ibelem commented Jun 13, 2024

@inexorabletash @huningxin @anssiko

Thanks for the review and feedback, removed monaco-editor and vite-plugin-monaco-editor npm packages, and fixed issues captured by @inexorabletash. PTAL.

Screenshot 2024-06-13 173749

Screenshot 2024-06-13 173800

@inexorabletash
Copy link
Member

Thanks - fixes look good. I appreciate the effort to make the simple HTTP server work! It's unfortunate to have to have a copy of the monaco editor in the source tree - is there any way to pull that in via npm to ease maintenance?

I'm on vacation this week so I'll give a quick LGTM to unblock.

Copy link
Member

@inexorabletash inexorabletash left a comment

Choose a reason for hiding this comment

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

Thank you!

@ibelem
Copy link
Contributor Author

ibelem commented Jun 14, 2024

@inexorabletash Wishing you a fantastic break!

The new commit removed monaco editor local codes from source tree, use the js files from CDN https://cdn.jsdelivr.net/npm/monaco-editor@0.49.0/ instead. @huningxin @anssiko PTAL

Verify it worked via python3 -m http.server 8888
http://localhost:8888/nnotepad/index.html

Verify it worked via npm run start
https://10.239.115.52:5173/nnotepad/index.html

Copy link
Member

@inexorabletash inexorabletash left a comment

Choose a reason for hiding this comment

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

One suggested change, but overall LGTM.

Thank you so much for taking this on!

.eslintignore Outdated Show resolved Hide resolved
@ibelem
Copy link
Contributor Author

ibelem commented Jun 18, 2024

@inexorabletash Thank you, added the /* global monaco */ instead of ignoring the whole file for eslint. PTAL.

Copy link
Member

@inexorabletash inexorabletash left a comment

Choose a reason for hiding this comment

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

LGTM!

@anssiko anssiko merged commit e1b2216 into webmachinelearning:master Jun 18, 2024
3 checks passed
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