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

feat(shield): Add A. Dux shield #951

Merged
merged 3 commits into from
Sep 23, 2021

Conversation

dxmh
Copy link
Collaborator

@dxmh dxmh commented Sep 22, 2021

This PR adds a new shield for Architeuthis Dux by Tapi (aka A. Dux, A.D., "Giant squid").

The shield has been available at dxmh/zmk-architeuthis-dux for a couple of months already, but it would be great for Architeuthis Dux to be supported in ZMK properly.

Wireless Architeuthis Dux with nice!nano controllers

@dxmh dxmh added enhancement New feature or request shields PRs and issues related to shields labels Sep 22, 2021
Copy link
Contributor

@petejohanson petejohanson left a comment

Choose a reason for hiding this comment

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

A few initial comments, thanks!

.github/workflows/build.yml Outdated Show resolved Hide resolved
@dxmh
Copy link
Collaborator Author

dxmh commented Sep 22, 2021

Thanks @petejohanson, I've updated the PR based on your comments. 👍

@caksoylar
Copy link
Contributor

caksoylar commented Sep 22, 2021

Would it be worth keeping the machine name a_dux similar to the QMK PR? It is pretty subjective, but might be user friendlier.

@petejohanson
Copy link
Contributor

Would it be worth keeping the machine name a_dux similar to the QMK PR? It is pretty subjective, but might be user friendlier.

Given most of the conversations on the LPK have always used the shorthand, and the QMK naming is also doing so, I'm leaning towards agreeing w/ this. And indeed, much easier to type as needed if editing build.yml files, etc.

Thoughts @dxmh?

@dxmh
Copy link
Collaborator Author

dxmh commented Sep 23, 2021

Thanks @caksoylar, @petejohanson - using the short name for user-friendliness is an excellent point.

I've just updated the PR so the shield now uses a_dux (and "A. Dux" for consistency).

@dxmh dxmh changed the title feat(shield): Add Architeuthis Dux shield (aka A. Dux) feat(shield): Add A. Dux shield Sep 23, 2021
The shorter name is more user-friendly.
Copy link
Contributor

@petejohanson petejohanson left a comment

Choose a reason for hiding this comment

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

Just one more minor fix found while verifying the metadata work.

Thanks!

app/boards/shields/a_dux/a_dux.zmk.yml Outdated Show resolved Hide resolved
Copy link
Contributor

@petejohanson petejohanson left a comment

Choose a reason for hiding this comment

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

LGTM!

@petejohanson petejohanson merged commit db4bbbf into zmkfirmware:main Sep 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request shields PRs and issues related to shields
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants