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

Add more layout for skiller_sgk50_s4 #24784

Open
wants to merge 8 commits into
base: develop
Choose a base branch
from

Conversation

itarze
Copy link
Contributor

@itarze itarze commented Jan 4, 2025

Description

Add more layout for skiller_sgk50_s4

Types of Changes

  • Core
  • Bugfix
  • New feature
  • Enhancement/optimization
  • Keyboard (addition or update)
  • Keymap/layout (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).

@itarze
Copy link
Contributor Author

itarze commented Jan 4, 2025

If review is OK, please merge as soon as possible, thanks.

@drashna drashna requested a review from a team January 4, 2025 04:23
@waffle87 waffle87 changed the base branch from master to develop January 6, 2025 09:32
@itarze
Copy link
Contributor Author

itarze commented Jan 22, 2025

@waffle87 Does this PR need to wait until the next Breaking Changes to be merged? Should it be merged into the develop branch? Looking forward to your reply, thank you.

@fauxpark
Copy link
Member

LAYOUT_all has overlapping keys.
image

@itarze
Copy link
Contributor Author

itarze commented Jan 22, 2025

@fauxpark This is due to the conflict between UK and US layouts. I’m not sure how to modify it. Could you assist me?

@fauxpark
Copy link
Member

fauxpark commented Jan 22, 2025

Are there two different PCBs for ANSI and ISO then? I would not expect, for example, the backspace area to consist of three separate matrix positions - usually one of them for the "split" backspace is wired to the 2u switch as well:
image
image

If so then this board should be split up into two revisions. That might be what needs to happen regardless.

@itarze
Copy link
Contributor Author

itarze commented Jan 22, 2025

@fauxpark The UK and US are on the same PCB.

@itarze
Copy link
Contributor Author

itarze commented Jan 22, 2025

@fauxpark Do you mean that this approach of combining multiple layouts on a single PCB cannot be merged?

@fauxpark
Copy link
Member

Not if there are overlapping keys. LAYOUT_all isn't recommended for use in the default keymap; it's meant to expose all possible matrix positions, regardless of whether it is physically possible. So you can either adjust the layout to eliminate overlap (in the backspace example, perhaps by changing the widths to 0.5u-1u-0.5u), or split up the board into ANSI and ISO variants.

@itarze
Copy link
Contributor Author

itarze commented Jan 22, 2025

@fauxpark please review it again. Thanks.

itarze and others added 2 commits January 23, 2025 06:41
Co-authored-by: Ryan <[email protected]>
Co-authored-by: Ryan <[email protected]>
@itarze itarze requested a review from fauxpark January 23, 2025 01:45
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.

LAYOUT_all still needs to be adjusted, as I mentioned above, or the board split into two revisions.

@itarze
Copy link
Contributor Author

itarze commented Jan 23, 2025

@fauxpark I have made the changes; please take another look.

@itarze itarze requested a review from fauxpark January 23, 2025 06:37
@fauxpark
Copy link
Member

Now there are no overlapping keys, but only because you've removed them. So they either never existed in the first place or you have misunderstood what I told you to do.
image

@itarze
Copy link
Contributor Author

itarze commented Jan 23, 2025

@fauxpark I’ve made further changes. Is it okay now?

@itarze itarze closed this Jan 23, 2025
@itarze itarze reopened this Jan 23, 2025
@fauxpark
Copy link
Member

image
Left Shift area still overlapping.

@itarze
Copy link
Contributor Author

itarze commented Jan 23, 2025

@fauxpark Has been modified.

@fauxpark
Copy link
Member

image
0.25u is maybe a little too small.

@itarze
Copy link
Contributor Author

itarze commented Jan 23, 2025

Where can I find the images of this layout? It would help me make adjustments on my own.

@fauxpark
Copy link
Member

You can hit Ctrl+Shift+I in the Configurator to load the keyboard.json for previewing the layout. Note that this does not allow you to compile the board, it's a developer tool only.

@itarze
Copy link
Contributor Author

itarze commented Jan 23, 2025

You can hit Ctrl+Shift+I in the Configurator to load the keyboard.json for previewing the layout. Note that this does not allow you to compile the board, it's a developer tool only.

I get it, Thanks. Is the current layout acceptable now?

@itarze

This comment was marked as spam.

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