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

Jump to bottom

Eek #529

Merged
merged 10 commits into from Jan 1, 2021
Merged

Eek #529

merged 10 commits into from Jan 1, 2021

Conversation

MangoIV
Copy link
Contributor

@MangoIV MangoIV commented Dec 19, 2020

  • added eek shield
  • added eek basic keymap
  • didn't change every commit since begin of the project :D

@joelspadin joelspadin added the shields PRs and issues related to shields label Dec 19, 2020
@innovaker innovaker added the enhancement New feature or request label Dec 20, 2020
Nicell
Nicell requested changes Dec 30, 2020
View reviewed changes
Copy link
Member

@Nicell Nicell left a comment

Choose a reason for hiding this comment

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

Thanks for adding support for this shield! All the DT stuff looks pretty solid.

There's a couple files that still need to be updated:

.github/workflows/build.yml: Add your board to the shield list.

docs/docs/hardware.md: You should add the eek! to this list

docs/static/setup.ps1 and docs/static/setup.sh: eek! should be added as a non-split option for both files.

@MangoIV
Copy link
Contributor Author

MangoIV commented Dec 31, 2020

Will do those tomorrow. Thanks.

@MangoIV
Copy link
Contributor Author

MangoIV commented Dec 31, 2020

added the eek! to the requested files. Thanks for reviewing!

Nicell
Nicell approved these changes Dec 31, 2020
View reviewed changes
Copy link
Member

@Nicell Nicell left a comment

Choose a reason for hiding this comment

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

Great! Thanks.

Nicell
Nicell requested changes Dec 31, 2020
View reviewed changes
Copy link
Member

@Nicell Nicell left a comment

Choose a reason for hiding this comment

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

Missed a couple license header spots. @MangoIV once these are fixed up I think this is good to go.

app/boards/shields/eek/Kconfig.defconfig
@@ -0,0 +1,6 @@
if SHIELD_EEK
Copy link
Member

Choose a reason for hiding this comment

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

Could you add a license header here?

app/boards/shields/eek/Kconfig.shield
@@ -0,0 +1,2 @@
config SHIELD_EEK
Copy link
Member

Choose a reason for hiding this comment

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

License header here, too, please! 😄

@MangoIV
Copy link
Contributor Author

MangoIV commented Dec 31, 2020

I just add "Copyright blabla " to every newly created file, yes?

@Nicell
Copy link
Member

Nicell commented Dec 31, 2020

I just add "Copyright blabla " to every newly created file, yes?

@MangoIV specifically the two I mentioned in the review, but in general, yes. The readme for example doesn't need it though.

@MangoIV
Copy link
Contributor Author

MangoIV commented Dec 31, 2020

should be fine now :)

@MangoIV
Copy link
Contributor Author

MangoIV commented Dec 31, 2020

sorry for the rough process, will be easier for the next shields :D

innovaker
innovaker reviewed Jan 1, 2021
View reviewed changes
Copy link
Member

@innovaker innovaker left a comment

Choose a reason for hiding this comment

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

Thanks! Very minor suggestions.

app/boards/shields/eek/eek.overlay Show resolved Hide resolved
app/boards/shields/eek/eek.overlay Show resolved Hide resolved
docs/docs/hardware.md Outdated
@@ -36,6 +36,7 @@ That being said, there are currently only a few specific [boards](/docs/faq#what
- [QAZ](https://www.cbkbd.com/product/qaz-keyboard-kit) (`qaz`)
- [CRBN](https://keygem.store/collections/group-buys/products/group-buy-featherlight-40-kit) (`crbn`)
- [tidbit](https://nullbits.co/tidbit/) (`tidbit`)
- [eek!](https://www.cbkbd.com/product/eek-keyboard) (`eek`)
Copy link
Member

Choose a reason for hiding this comment

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

You added a ! to the keyboard name setup descriptions. Should this link also follow that style?

Also, I noticed you used a capital "Eek!" in setup and "eek!" in the keyboard name.

app/boards/shields/eek/eek.overlay Outdated
compatible = "zmk,kscan-gpio-matrix";
label = "KSCAN";
diode-direction = "col2row";
Copy link
Member

Choose a reason for hiding this comment

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

The indentation here looks off?

@MangoIV
Copy link
Contributor Author

MangoIV commented Jan 1, 2021

Should be fine now?

@innovaker innovaker merged commit 5752b4f into zmkfirmware:main Jan 1, 2021
1 check passed
jmding8 pushed a commit to jmding8/zmk that referenced this pull request Jan 15, 2021
@MangoIV @jmding8
feat(shields): add eek shield
3b0c423 
PR: zmkfirmware#529
tyalie pushed a commit to tyalie/zmk that referenced this pull request Nov 15, 2022
@MangoIV
feat(shields): add eek shield
43fa448 
PR: zmkfirmware#529
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Nicell Nicell Nicell approved these changes

@innovaker innovaker innovaker approved these changes

Assignees

@MangoIV MangoIV

@innovaker innovaker

Labels
enhancement New feature or request shields PRs and issues related to shields
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@MangoIV @Nicell @innovaker @joelspadin