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

Adding afternoonlabs/oceanbreeze #12299

Merged
merged 10 commits into from
Apr 10, 2021
Merged

Adding afternoonlabs/oceanbreeze #12299

merged 10 commits into from
Apr 10, 2021

Conversation

eithanshavit
Copy link

Description

Adding the new Ocean Breeze keyboard to Afternoon Labs

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: C, Python
  • I have read the PR Checklist document and have made the appropriate changes.
  • 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).

@eithanshavit
Copy link
Author

@fauxpark and @drashna, you both helped me review stuff before. Looking for some feedback. Thx!

Copy link
Member

@fauxpark fauxpark left a comment

Choose a reason for hiding this comment

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

Looks good, except the info.json needs to be redone without the rotation:
image

@fauxpark fauxpark requested a review from a team March 24, 2021 05:02
@eithanshavit
Copy link
Author

@fauxpark, I should probably know this by now, but how are you testing these info.json files? Where's the render you're showing from?

@fauxpark
Copy link
Member

fauxpark commented Mar 24, 2021

You can preview info.json files by pressing Ctrl+Shift+I in the Configurator. It's also a good idea to verify the order matches the LAYOUT macro by turning on Fast Input and spamming a key.

@eithanshavit
Copy link
Author

Thanks so much @fauxpark, I fixed the layout!

@eithanshavit eithanshavit requested review from fauxpark and removed request for a team March 25, 2021 00:08
@drashna drashna requested a review from a team April 3, 2021 02:00
@eithanshavit
Copy link
Author

eithanshavit commented Apr 4, 2021

@fauxpark, I think I finally got it :) Sorry for the delay. This is the order I think makes sense for this layout.

Screen Shot 2021-04-03 at 10 17 36 PM

@fauxpark
Copy link
Member

fauxpark commented Apr 4, 2021

Per the layout macro:
image

I think it should be more like this:
image

@eithanshavit
Copy link
Author

Thanks @fauxpark, is there a requirement that the macro and the info.json will follow the same order? I think that my proposal right now makes sense from the eyes of a user of this keyboard. For the macro, it makes sense for someone configuring this layout visually per the macro layout. For the info.json layout and configurator, the current proposal makes sense because configuring the thumb clusters is a different process than the alphas. And so I don't necessarily agree with your latest screenshot of having the upper thumb keys be in order of the lower alpha row.

@fauxpark
Copy link
Member

fauxpark commented Apr 4, 2021

The info.json determines what order the keys will be placed into the top half of the layout macro, and so they must exactly match. You've placed LT4 and RT1 in the lower alpha row of the layout macro, which is about where they are physically, thus they have to be placed there in the info.json. If you want your ordering, you'll need to shift those four thumb keys down a line in the layout macro.

@eithanshavit
Copy link
Author

I see, ok I've updated it to behave like this.
Screen Shot 2021-04-03 at 10 55 57 PM

@eithanshavit
Copy link
Author

How's this looking now @fauxpark? Thanks for all the guidance

@eithanshavit eithanshavit requested review from fauxpark and removed request for a team April 5, 2021 02:49
Copy link
Member

@fauxpark fauxpark left a comment

Choose a reason for hiding this comment

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

Looks pretty good, just a couple of minor cosmetic things.

keyboards/afternoonlabs/oceanbreeze/rev1/rules.mk Outdated Show resolved Hide resolved
keyboards/afternoonlabs/oceanbreeze/rev1/rev1.h Outdated Show resolved Hide resolved
@eithanshavit eithanshavit requested a review from fauxpark April 5, 2021 23:46
@fauxpark fauxpark requested a review from a team April 6, 2021 04:23
@eithanshavit
Copy link
Author

Looking for more reviews please 🙏

@drashna
Copy link
Member

drashna commented Apr 10, 2021

Thanks!

@drashna drashna merged commit e09ca63 into qmk:master Apr 10, 2021
makenova pushed a commit to makenova/qmk_firmware that referenced this pull request Apr 26, 2021
rizo pushed a commit to rizo/qmk_firmware that referenced this pull request May 10, 2021
toddyamakawa pushed a commit to toddyamakawa/qmk_firmware that referenced this pull request May 19, 2021
HokieGeek pushed a commit to HokieGeek/qmk_firmware that referenced this pull request Jul 11, 2021
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.

3 participants