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

[Keyboard] Add Flatbread60, Vaneela, and VaneelaEx by nckiibs #9817

Merged
merged 34 commits into from
Aug 9, 2020

Conversation

noclew
Copy link
Contributor

@noclew noclew commented Jul 25, 2020

Description

Types of Changes

  • Core
  • Bugfix
  • New feature
  • Enhancement/optimization
  • Keyboard (addition or update)
  • Keymap/layout/userspace (addition or update)
  • Documentation

Issues Fixed or Closed by This PR

Checklist

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • I have tested the changes and verified that they work and don't break anything (as well as I can manage).

@zvecr zvecr added the keyboard label Jul 26, 2020
@zvecr zvecr changed the title A new keyboard flatbread60 has been added [keyboard] Add flatbread60 support Jul 26, 2020
keyboards/nckiibs/flatbread60/keymaps/default/readme.md Outdated Show resolved Hide resolved
keyboards/nckiibs/flatbread60/rules.mk Outdated Show resolved Hide resolved
keyboards/nckiibs/flatbread60/readme.md Outdated Show resolved Hide resolved
keyboards/nckiibs/flatbread60/readme.md Outdated Show resolved Hide resolved
keyboards/nckiibs/flatbread60/info.json Outdated Show resolved Hide resolved
keyboards/nckiibs/flatbread60/config.h Outdated Show resolved Hide resolved
keyboards/nckiibs/flatbread60/config.h Outdated Show resolved Hide resolved
keyboards/nckiibs/flatbread60/config.h Outdated Show resolved Hide resolved
@noclew
Copy link
Contributor Author

noclew commented Jul 30, 2020

Thank you very much for your help! I fixed the issues pointed out.

@noclew
Copy link
Contributor Author

noclew commented Jul 30, 2020

I also added two more keyboards. Do I have to close and make another pull request?

@noclew
Copy link
Contributor Author

noclew commented Jul 31, 2020

All done!

keyboards/nckiibs/flatbread60/config.h Outdated Show resolved Hide resolved
keyboards/nckiibs/flatbread60/keymaps/default/keymap.c Outdated Show resolved Hide resolved
keyboards/nckiibs/vaneela/keymaps/via/keymap.c Outdated Show resolved Hide resolved
keyboards/nckiibs/vaneelaex/keymaps/via/keymap.c Outdated Show resolved Hide resolved
keyboards/nckiibs/vaneelaex/vaneelaex.h Show resolved Hide resolved
@drashna drashna requested a review from a team August 4, 2020 04:00
keyboards/nckiibs/flatbread60/keymaps/default/keymap.c Outdated Show resolved Hide resolved
keyboards/nckiibs/flatbread60/keymaps/default/keymap.c Outdated Show resolved Hide resolved
keyboards/nckiibs/flatbread60/keymaps/default/keymap.c Outdated Show resolved Hide resolved
keyboards/nckiibs/flatbread60/keymaps/default/keymap.c Outdated Show resolved Hide resolved
keyboards/nckiibs/flatbread60/keymaps/default/keymap.c Outdated Show resolved Hide resolved
keyboards/nckiibs/flatbread60/keymaps/default/keymap.c Outdated Show resolved Hide resolved
keyboards/nckiibs/flatbread60/keymaps/default/keymap.c Outdated Show resolved Hide resolved
keyboards/nckiibs/flatbread60/keymaps/default/keymap.c Outdated Show resolved Hide resolved
keyboards/nckiibs/flatbread60/keymaps/default/keymap.c Outdated Show resolved Hide resolved
keyboards/nckiibs/flatbread60/keymaps/default/keymap.c Outdated Show resolved Hide resolved
Copy link
Member

@noroadsleft noroadsleft left a comment

Choose a reason for hiding this comment

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

In addition to the specific suggestions, I'm not keen on the fact that the default and via keymaps for each board don't use the same mappings. This can be disorienting to users who switch between VIA and non-VIA firmware, and IMO these keymaps should be as similar as possible. I may return with more suggestions along these lines later.

GitHub tip: Multiple suggestions can be applied to a single commit through the Files Changed tab.

keyboards/nckiibs/flatbread60/config.h Show resolved Hide resolved
keyboards/nckiibs/flatbread60/keymaps/default/keymap.c Outdated Show resolved Hide resolved
keyboards/nckiibs/flatbread60/keymaps/default/readme.md Outdated Show resolved Hide resolved
keyboards/nckiibs/flatbread60/readme.md Outdated Show resolved Hide resolved
keyboards/nckiibs/vaneela/config.h Show resolved Hide resolved
keyboards/nckiibs/vaneela/readme.md Outdated Show resolved Hide resolved
keyboards/nckiibs/vaneelaex/config.h Show resolved Hide resolved
keyboards/nckiibs/vaneelaex/info.json Outdated Show resolved Hide resolved
keyboards/nckiibs/vaneelaex/readme.md Outdated Show resolved Hide resolved
@noclew
Copy link
Contributor Author

noclew commented Aug 5, 2020

In addition to the specific suggestions, I'm not keen on the fact that the default and via keymaps for each board don't use the same mappings. This can be disorienting to users who switch between VIA and non-VIA firmware, and IMO these keymaps should be as similar as possible. I may return with more suggestions along these lines later.

GitHub tip: Multiple suggestions can be applied to a single commit through the Files Changed tab.

Thank you! I fixed what were requested. By the way, what is the proper response to the change requests? Do I have to click every 'resolve conversation' button after fixing them?

keyboards/nckiibs/flatbread60/keymaps/default/keymap.c Outdated Show resolved Hide resolved
keyboards/nckiibs/flatbread60/keymaps/via/keymap.c Outdated Show resolved Hide resolved
keyboards/nckiibs/vaneelaex/keymaps/default/keymap.c Outdated Show resolved Hide resolved
keyboards/nckiibs/vaneelaex/keymaps/default/keymap.c Outdated Show resolved Hide resolved
keyboards/nckiibs/vaneelaex/keymaps/default/keymap.c Outdated Show resolved Hide resolved
keyboards/nckiibs/vaneelaex/keymaps/default/keymap.c Outdated Show resolved Hide resolved
keyboards/nckiibs/vaneelaex/keymaps/via/keymap.c Outdated Show resolved Hide resolved
keyboards/nckiibs/vaneelaex/keymaps/via/keymap.c Outdated Show resolved Hide resolved
keyboards/nckiibs/vaneelaex/keymaps/via/keymap.c Outdated Show resolved Hide resolved
keyboards/nckiibs/vaneelaex/keymaps/via/keymap.c Outdated Show resolved Hide resolved
@noclew noclew requested a review from fauxpark August 6, 2020 05:39
@noroadsleft
Copy link
Member

Thank you! I fixed what were requested. By the way, what is the proper response to the change requests? Do I have to click every 'resolve conversation' button after fixing them?

Depends how you apply the suggestions. If you apply them through GitHub's web interface, they should resolve automatically. If you make the changes locally and then push them to your fork, you do need to resolve them manually.

With a lot of suggestions, I'd suggest using the Files Changed tab to make them, then when you have them committed on GitHub, use git pull origin nckiibs to pull those changes to the source branch on your local repo (the files on your computer). That will automatically resolve the suggestions, but keep your fork and your local repository synchronized. Note that you'll need to have your local nckiibs branch checked out and clean (no modifications) for this to work without any hiccups/issues.

keyboards/nckiibs/flatbread60/keymaps/default/keymap.c Outdated Show resolved Hide resolved
keyboards/nckiibs/flatbread60/keymaps/via/keymap.c Outdated Show resolved Hide resolved
keyboards/nckiibs/vaneela/keymaps/default/keymap.c Outdated Show resolved Hide resolved
keyboards/nckiibs/vaneela/keymaps/via/keymap.c Outdated Show resolved Hide resolved
keyboards/nckiibs/vaneelaex/keymaps/default/keymap.c Outdated Show resolved Hide resolved
keyboards/nckiibs/vaneelaex/keymaps/via/keymap.c Outdated Show resolved Hide resolved
@noclew
Copy link
Contributor Author

noclew commented Aug 6, 2020

Thank you! I fixed what were requested. By the way, what is the proper response to the change requests? Do I have to click every 'resolve conversation' button after fixing them?

Depends how you apply the suggestions. If you apply them through GitHub's web interface, they should resolve automatically. If you make the changes locally and then push them to your fork, you do need to resolve them manually.

With a lot of suggestions, I'd suggest using the Files Changed tab to make them, then when you have them committed on GitHub, use git pull origin nckiibs to pull those changes to the source branch on your local repo (the files on your computer). That will automatically resolve the suggestions, but keep your fork and your local repository synchronized. Note that you'll need to have your local nckiibs branch checked out and clean (no modifications) for this to work without any hiccups/issues.

@noroadsleft @drashna
Thank you for the information. I am learning this review process, so it will be appreciated if you can answer these noob questions!

  1. Upon the change requests by reviewers, is there a way to filter the files that were quested to change in the Files Changed tab? Last time, I went there, added changes to the batch, ran batch commits, and marked all files as viewed. After the new changes were requested, I checked the Files Changed tab again, and because all the files were still marked as 'viewed', there was no way to filter out the files that the new changes have been requested with. In this process, is there a way to quickly filter out the files with new change requests? Or, do I have to go through every file again and check the red box to find the new change requests?

  2. Once I changed the files requested and batch committed them on Github, I more than often have failures in the automated checks because of the files of other keyboards. This time, it is PR Lint keyboards check, and it looks like there are some errors with the files of other keyboards, specifically, Run qmk info test with Kudox keyboard. If this happens, what I usually do is 1) pull the lastest Github batch commit to my local repo 2) rebase the local repo with the latest QMK build 3) commit and push the local repo to Github and 4) wait to see if it passes the two automated checks. Is this procedure correct to deal with the automated check failures? Sometimes this does not work when the official QMK repo is not updated and is still with errors, so I feel like this is not the correct way to deal with the failures in automated checks from others' files. Can you let me know what is the proper procedure?

  3. I am a bit confused by VIA keymaps. @drashna told me that functions for switching layers are not necessary because VIA has its own. I looked at others' file, and it seems like they do not define custom functions to switch layers and only have the keymaps. However, in the latest change request, it was asked to put the functions back to the keymap files, so I wonder what is the right way to write the VIA kemaps.

I am sorry for the long questions, and I really appreciate all the efforts to help newbies in the community!

@noroadsleft
Copy link
Member

  1. Upon the change requests by reviewers, is there a way to filter the files that were quested to change in the Files Changed tab? Last time, I went there, added changes to the batch, ran batch commits, and marked all files as viewed. After the new changes were requested, I checked the Files Changed tab again, and because all the files were still marked as 'viewed', there was no way to filter out the files that the new changes have been requested with. In this process, is there a way to quickly filter out the files with new change requests? Or, do I have to go through every file again and check the red box to find the new change requests?

I don't have an answer for this one; I'll probably have to look into it. I know what things are likely to be requested due to being a collaborator, so I typically jump ahead on these things before opening a PR.

  1. Once I changed the files requested and batch committed them on Github, I more than often have failures in the automated checks because of the files of other keyboards. This time, it is PR Lint keyboards check, and it looks like there are some errors with the files of other keyboards, specifically, Run qmk info test with Kudox keyboard. If this happens, what I usually do is 1) pull the lastest Github batch commit to my local repo 2) rebase the local repo with the latest QMK build 3) commit and push the local repo to Github and 4) wait to see if it passes the two automated checks. Is this procedure correct to deal with the automated check failures? Sometimes this does not work when the official QMK repo is not updated and is still with errors, so I feel like this is not the correct way to deal with the failures in automated checks from others' files. Can you let me know what is the proper procedure?

The PR Lint check is literally a "first-effort" – if it's not flagging boards you've changed, ignore it.

  1. I am a bit confused by VIA keymaps. @drashna told me that functions for switching layers are not necessary because VIA has its own. I looked at others' file, and it seems like they do not define custom functions to switch layers and only have the keymaps. However, in the latest change request, it was asked to put the functions back to the keymap files, so I wonder what is the right way to write the VIA kemaps.

VIA's functions for this only work if you use the keycodes VIA provides for doing this, and they also only work if you have exactly four layers.

@noclew noclew requested a review from noroadsleft August 8, 2020 21:29
keyboards/nckiibs/flatbread60/keymaps/via/keymap.c Outdated Show resolved Hide resolved
keyboards/nckiibs/vaneela/keymaps/via/keymap.c Outdated Show resolved Hide resolved
keyboards/nckiibs/vaneelaex/keymaps/via/keymap.c Outdated Show resolved Hide resolved
@noroadsleft noroadsleft changed the title [keyboard] Add flatbread60 support [Keyboard] Add Flatbread60, Vaneela, and VaneelaEx by nckiibs Aug 9, 2020
@noclew noclew requested a review from noroadsleft August 9, 2020 05:56
@noroadsleft noroadsleft merged commit 98e1e18 into qmk:master Aug 9, 2020
@noroadsleft
Copy link
Member

Thanks!

@noclew
Copy link
Contributor Author

noclew commented Aug 9, 2020 via email

@noclew
Copy link
Contributor Author

noclew commented Aug 9, 2020 via email

fdawans pushed a commit to fdawans/qmk_firmware that referenced this pull request Aug 11, 2020
* creOnic added

* made requested changed by a moderator

* device name changed

* fixed flatbread60 files

* add vaneela and vaneelaEX support

* Update keyboards/nckiibs/vaneelaex/rules.mk

* vaneela rgb disabled

* include error fxied

* vaneelaex Via keymap fixed

* vaneelaex keymap fixed

* all fixed except clang part

* vaneelaex config.h error fixed, clang fixed

* Update keyboards/nckiibs/flatbread60/keymaps/default/keymap.c

* Update keyboards/nckiibs/flatbread60/keymaps/default/keymap.c

done

* Update keyboards/nckiibs/flatbread60/keymaps/default/keymap.c

done

* Update keyboards/nckiibs/flatbread60/keymaps/default/keymap.c

done

* Update keyboards/nckiibs/flatbread60/keymaps/default/keymap.c

done

* Update keyboards/nckiibs/flatbread60/keymaps/default/keymap.c

* Update keyboards/nckiibs/flatbread60/keymaps/default/keymap.c

* Update keyboards/nckiibs/flatbread60/keymaps/default/keymap.c

* Update keyboards/nckiibs/flatbread60/keymaps/default/keymap.c

* Update keyboards/nckiibs/flatbread60/keymaps/default/keymap.c

* Update keyboards/nckiibs/flatbread60/rules.mk

done

* line endings were fixed as requested

* Apply suggestions from code review

Thank you!

* requests applied and rebased

* pics changed

* flatbread via fixed

* Apply suggestions from code review

Thanks!

* Apply suggestions from code review

Done!

* Apply suggestions from code review except VIA keymaps

Questions regarding VIA keymaps are pending

* Update keyboards/nckiibs/flatbread60/keymaps/default/keymap.c

* flatbread via keymap changed

* Apply suggestions from code review
fdawans added a commit to fdawans/qmk_firmware that referenced this pull request Aug 11, 2020
nicocesar pushed a commit to nicocesar/qmk_firmware that referenced this pull request Aug 12, 2020
* creOnic added

* made requested changed by a moderator

* device name changed

* fixed flatbread60 files

* add vaneela and vaneelaEX support

* Update keyboards/nckiibs/vaneelaex/rules.mk

* vaneela rgb disabled

* include error fxied

* vaneelaex Via keymap fixed

* vaneelaex keymap fixed

* all fixed except clang part

* vaneelaex config.h error fixed, clang fixed

* Update keyboards/nckiibs/flatbread60/keymaps/default/keymap.c

* Update keyboards/nckiibs/flatbread60/keymaps/default/keymap.c

done

* Update keyboards/nckiibs/flatbread60/keymaps/default/keymap.c

done

* Update keyboards/nckiibs/flatbread60/keymaps/default/keymap.c

done

* Update keyboards/nckiibs/flatbread60/keymaps/default/keymap.c

done

* Update keyboards/nckiibs/flatbread60/keymaps/default/keymap.c

* Update keyboards/nckiibs/flatbread60/keymaps/default/keymap.c

* Update keyboards/nckiibs/flatbread60/keymaps/default/keymap.c

* Update keyboards/nckiibs/flatbread60/keymaps/default/keymap.c

* Update keyboards/nckiibs/flatbread60/keymaps/default/keymap.c

* Update keyboards/nckiibs/flatbread60/rules.mk

done

* line endings were fixed as requested

* Apply suggestions from code review

Thank you!

* requests applied and rebased

* pics changed

* flatbread via fixed

* Apply suggestions from code review

Thanks!

* Apply suggestions from code review

Done!

* Apply suggestions from code review except VIA keymaps

Questions regarding VIA keymaps are pending

* Update keyboards/nckiibs/flatbread60/keymaps/default/keymap.c

* flatbread via keymap changed

* Apply suggestions from code review
@noroadsleft
Copy link
Member

Oh, by the way, I just checked qmk configurator, and it seems like the default keymap was no registered. How can I make it so that the there shows a default keymap in QMK configurator?

@noclew,

QMK Configurator keymaps live here: https://github.com/qmk/qmk_configurator/tree/master/public/keymaps

Instructions: https://github.com/qmk/qmk_firmware/blob/633d0fe5f7eef8400cfdc843f1a043d0eac8a836/docs/configurator_default_keymaps.md (hopefully this will hit the QMK docs soon)

fcoury pushed a commit to fcoury/qmk_firmware_archive that referenced this pull request Sep 20, 2020
* creOnic added

* made requested changed by a moderator

* device name changed

* fixed flatbread60 files

* add vaneela and vaneelaEX support

* Update keyboards/nckiibs/vaneelaex/rules.mk

* vaneela rgb disabled

* include error fxied

* vaneelaex Via keymap fixed

* vaneelaex keymap fixed

* all fixed except clang part

* vaneelaex config.h error fixed, clang fixed

* Update keyboards/nckiibs/flatbread60/keymaps/default/keymap.c

* Update keyboards/nckiibs/flatbread60/keymaps/default/keymap.c

done

* Update keyboards/nckiibs/flatbread60/keymaps/default/keymap.c

done

* Update keyboards/nckiibs/flatbread60/keymaps/default/keymap.c

done

* Update keyboards/nckiibs/flatbread60/keymaps/default/keymap.c

done

* Update keyboards/nckiibs/flatbread60/keymaps/default/keymap.c

* Update keyboards/nckiibs/flatbread60/keymaps/default/keymap.c

* Update keyboards/nckiibs/flatbread60/keymaps/default/keymap.c

* Update keyboards/nckiibs/flatbread60/keymaps/default/keymap.c

* Update keyboards/nckiibs/flatbread60/keymaps/default/keymap.c

* Update keyboards/nckiibs/flatbread60/rules.mk

done

* line endings were fixed as requested

* Apply suggestions from code review

Thank you!

* requests applied and rebased

* pics changed

* flatbread via fixed

* Apply suggestions from code review

Thanks!

* Apply suggestions from code review

Done!

* Apply suggestions from code review except VIA keymaps

Questions regarding VIA keymaps are pending

* Update keyboards/nckiibs/flatbread60/keymaps/default/keymap.c

* flatbread via keymap changed

* Apply suggestions from code review
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants