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

docs: update README with overview and instructions #114

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

joeynguyen
Copy link

@joeynguyen joeynguyen commented Dec 21, 2024

I noticed the README file is pretty bare. You may be working on updating it already so feel free to close this PR if you already have one prepared. But if you didn't, I figured I could save you some time by generating (Copilot did most of the work although it did hallucinate and wrote that the project contained an MIT license 😂) a basic one which you can improve on. It includes steps to help others that are new to the project get started.

Copy link

netlify bot commented Dec 21, 2024

Deploy Preview for zmk-studio ready!

Name Link
🔨 Latest commit 9f00958
🔍 Latest deploy log https://app.netlify.com/sites/zmk-studio/deploys/6769f727c664ce0008451443
😎 Deploy Preview https://deploy-preview-114.preview.zmk.studio
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

README.md Outdated
Comment on lines 24 to 25
- [Node.js](https://nodejs.org/) (version 14 or later)
- [npm](https://www.npmjs.com/) (version 6 or later)
Copy link

Choose a reason for hiding this comment

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

thought: it'd be great if we recommend the latest LTS.

Copy link
Author

Choose a reason for hiding this comment

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

good call, I've updated that in caed589

README.md Outdated
1. **Run Tauri development server**:

```sh
npm run tauri dev
Copy link

Choose a reason for hiding this comment

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

issue: this will not work, it should be npm run tauri -- dev 😃

Copy link
Author

Choose a reason for hiding this comment

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

are you sure it won't work? It seems to work for me on the main branch.

image

Copy link
Author

Choose a reason for hiding this comment

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

but I'm new to Tauri so if -- dev is the preferred approach, I'm happy to update that as well. So npm run tauri dev doesn't work on your machine when you try it?

I'm on MacOS btw. Curious which OS you're running if it doesn't work for you?

Copy link
Author

Choose a reason for hiding this comment

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

Actually you're right, it's not a tauri convention but an npm one for passing arguments to the command. I don't understand why my command was working (even npm run tauri build works) but what you're suggesting does seem like the more correct syntax.

Updated in 1ff1c51

Copy link

@pongstr pongstr Dec 22, 2024

Choose a reason for hiding this comment

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

i'm on mac as well, tried again and it works. i wonder why it didn't in the beginning. you can ignore my comment.

edit:
i don't use npm these days (a lot of my projects are on pnpm), it might have been due to an older version? (not sure), i upgraded to 11.0.0 and it works with npm run tauri dev 🤷🏼 ... it wouldn't hurt to recommend -- imho, it's also stated in the npm docs

Copy link
Member

Choose a reason for hiding this comment

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

as a counterpoint, tauri docs omit the --. I don't think it matters too much though

README.md Outdated
Comment on lines 9 to 12
- **Graphical Keymap Editor**: Easily create and modify keymaps with a visual editor.
- **Device Management**: Connect to your keyboard via USB or Bluetooth and manage its settings.
- **Firmware Updates**: Flash new firmware to your device directly from the application.
- **Real-time Feedback**: Receive real-time notifications and feedback from your device.
Copy link
Member

Choose a reason for hiding this comment

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

This section is kinda confusing to me. Currently it only really supports real-time keymap updating. There is no device management at the moment. These updates are done via RPC + settings system, there's no flashing being done. And I don't know what real-time notifications there are. Was this ChatGPT'd by chance 🫣

Copy link
Author

@joeynguyen joeynguyen Dec 23, 2024

Choose a reason for hiding this comment

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

Hey @Nicell, yes thanks for catching that and yes you're totally right that it was done by an LLM. I was trying to be transparent about that in the PR description using the word "generating" instead of "writing" and also mentioning that, "Copilot did most of the work". I'm not a subject matter expert on ZMK Studio so was relying on the LLM to do a better job than I could with fleshing out the README and was hoping for experts like yourself and @pongstr to catch mistakes as part of the PR. I apologize for any misunderstandings or if you're against the use of LLMs.

So your suggestion, is to keep just the first bullet (Graphical Keymap Editor) and delete the other 3 (Device Management, Firmware Updates, Real-time Feedback)? I can go ahead and remove those and add any additional bullets you think would be useful.

Btw, unrelated but I'm a big fan of the Nice!Nano and Typeractive website. I'm using the Nano and View on my Kyria, and I especially like the 3D Corne builder preview on Typeractive. Have bought a few things on there in the past (the lithium batteries are great!).

Copy link
Author

Choose a reason for hiding this comment

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

remove hallucinated features in 9f00958

README.md Outdated
- [Node.js](https://nodejs.org/) (version 22 or later)
- [npm](https://www.npmjs.com/) (version 10 or later)
- [Rust](https://www.rust-lang.org/tools/install) (for building Tauri applications)
- [Tauri CLI](https://tauri.app/v1/guides/getting-started/prerequisites/)
Copy link
Member

Choose a reason for hiding this comment

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

Let's link to v2 since that's what we use

Suggested change
- [Tauri CLI](https://tauri.app/v1/guides/getting-started/prerequisites/)
- [Tauri CLI](https://v2.tauri.app/start/prerequisites/)

Also debatably don't need to list Rust then since Tauri's install pre-reqs have it listed... And then also maybe don't need to mention npm since Node.js installs it by default.

Copy link
Author

Choose a reason for hiding this comment

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

done in 32c8fa4

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.

3 participants