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

improve: Key Component scale transition #108

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

pongstr
Copy link

@pongstr pongstr commented Dec 12, 2024

  • improve: smoother scale transition when hovering
  • feat: toggle select+deselect, allows users to deselect a currently selected key

Storybook before+after

before after
Before.mov
After.mov

desktop build (macos)

Screen.Recording.2024-12-12.at.15.39.47.mov

Copy link

netlify bot commented Dec 12, 2024

Deploy Preview for zmk-studio ready!

Name Link
🔨 Latest commit 26486af
🔍 Latest deploy log https://app.netlify.com/sites/zmk-studio/deploys/676897d5163f7e0008abb6f2
😎 Deploy Preview https://deploy-preview-108.preview.zmk.studio
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@pongstr pongstr force-pushed the refactor-key-component branch 2 times, most recently from 7d133d6 to e40af6e Compare December 12, 2024 13:49
@pongstr pongstr marked this pull request as ready for review December 12, 2024 14:02
@pongstr pongstr force-pushed the refactor-key-component branch from e40af6e to 2669d02 Compare December 12, 2024 20:04
@filiptdz
Copy link

Solves the issues described in #99 and #98

src/keyboard/Key.tsx Outdated Show resolved Hide resolved
Copy link
Collaborator

@petejohanson petejohanson left a comment

Choose a reason for hiding this comment

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

The UI itself seems way better! See my one code comment, but I'm optimistic about this. I'm not sure about the willChange tweak, given that did improve the rendering for some folks, so I'm uncertain about that particular tweak without broader testing/feedback across more browsers/platforms.

src/keyboard/Key.tsx Show resolved Hide resolved
@pongstr pongstr force-pushed the refactor-key-component branch from 2669d02 to 55f5f8e Compare December 20, 2024 21:33
- improve: smoother scale transition when hovering
- feat: toggle select+deselect, allows users to deselect a currently
  selected key

.
@pongstr pongstr force-pushed the refactor-key-component branch from 55f5f8e to e3ff99c Compare December 20, 2024 21:38
@pongstr
Copy link
Author

pongstr commented Dec 21, 2024

I'm not sure about the willChange tweak, given that did improve the rendering for some folks

will-change might not be an ideal solution, especially when applied to frequently repeated elements (like <Key/>), as it can significantly increase resource usage.

I'm uncertain about that particular tweak without broader testing/feedback across more browsers/platforms.

I've tested this proposal on

  • desktop app on macOS
  • web app on macOS and windows

the improvements are apparent, i'll test both on my Debian machine tomorrow.

btw, it'd be nice if PR can also have desktop-app build pipeline to make proposals accessible for testing, setting up tauri on windows is quite laborious 😄

Anyway, let me know if there's anything else I can do to get this PR to move forward. Thanks!

@pongstr pongstr requested a review from petejohanson December 21, 2024 00:20
src/keyboard/Key.tsx Show resolved Hide resolved
src/keyboard/Key.tsx Outdated Show resolved Hide resolved
src/keyboard/Key.tsx Outdated Show resolved Hide resolved
src/keyboard/Keymap.tsx Show resolved Hide resolved
@pongstr pongstr requested a review from Nicell December 22, 2024 20:02
src/keyboard/Key.tsx Outdated Show resolved Hide resolved
@pongstr pongstr requested a review from Nicell December 22, 2024 22:53
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.

Just one last thing I see!

>
{header && (
<span className="p-0 b-0 m-0 text-xs w-full h-full text-nowrap justify-self-start row-start-1 row-end-2 col-start-1 col-end-4 hidden group-hover:inline-block group-hover:truncate @md:underline">
<span className="w-9/12 absolute text-micro top-1 left-1 right-1 leading-none text-cemter group-hover:block uppercase h-4 hidden truncate whitespace-nowrap">
Copy link
Member

Choose a reason for hiding this comment

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

text-cemter 😅

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

Successfully merging this pull request may close these issues.

4 participants