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

Fix configuration data for keyboard pom_keyboards/tnln95 #9904

Merged
merged 4 commits into from
Aug 6, 2020

Conversation

NMHoang05
Copy link
Contributor

@NMHoang05 NMHoang05 commented Aug 1, 2020

I checked my keyboard layout on configurator and it doesn't work as intended.

Description

I update the info file.
I commented out layout "all" to avoid confusion in the configurator (it appears despite not declared in the info file).
I add layout all to info file
I rearrange the layout "ansi".

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).

keyboards/pom_keyboards/tnln95/tnln95.h Outdated Show resolved Hide resolved
@noroadsleft noroadsleft self-requested a review August 1, 2020 19:06
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.

The changes to the file contents themselves look good, but technically speaking, the default keymap should be named default_ansi and the default_full keymap should be named default.

https://docs.qmk.fm/#/hardware_keyboard_guidelines?id=ltkeyboard_namehgt, Paragraph 3:

When defining multiple layouts you should have a base layout, named LAYOUT_all, that supports all possible switch positions on your matrix, even if that layout is impossible to build physically. This is the macro you should use in your default keymap. You should then have additional keymaps named default_<layout> that use your other layout macros. This will make it easier for people to use the layouts you define.

@noroadsleft
Copy link
Member

@NMHoang05,

I feel like I should apologize for this trouble. I knew that the info.json was incorrect on #9788, but it was messed up in a way where I knew it'd be easier to fix it if I had an existing implementation in place. I was going to do that this morning, but you beat me to it. (QMK Configurator implementations are my area of expertise here.)

@NMHoang05
Copy link
Contributor Author

NMHoang05 commented Aug 2, 2020

@noroadsleft
no worries 👍
I did copy/paste the info.json and some parts weren't properly modified.

I have renamed the keymap.

@noroadsleft noroadsleft merged commit 1b4f47c into qmk:master Aug 6, 2020
@noroadsleft
Copy link
Member

Thanks!

fdawans pushed a commit to fdawans/qmk_firmware that referenced this pull request Aug 11, 2020
* fix configuration data, comment out unused layout

* fix keymap

* add info for layout all

* change keymap name
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
* fix configuration data, comment out unused layout

* fix keymap

* add info for layout all

* change keymap name
fcoury pushed a commit to fcoury/qmk_firmware_archive that referenced this pull request Sep 20, 2020
* fix configuration data, comment out unused layout

* fix keymap

* add info for layout all

* change keymap name
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants