Skip to content

feat: Steppers with shift +/-10 modifiers for number inputs#26

Open
aniforprez wants to merge 3 commits intorockfactory:devfrom
aniforprez:feat/number-input-controls
Open

feat: Steppers with shift +/-10 modifiers for number inputs#26
aniforprez wants to merge 3 commits intorockfactory:devfrom
aniforprez:feat/number-input-controls

Conversation

@aniforprez
Copy link
Copy Markdown
Contributor

Number inputs for resources now have the steppers added back and will increment/decrement by 10 when shift is pressed.

Screen.Recording.2024-11-11.at.7.18.30.PM.mov

rockfactory and others added 3 commits November 10, 2024 23:08
Added a new wrapper around the NumberInput that listens to shift up/down
events and sets the step to +10 instead of +1 when shift is held down
}>();
useEffect(() => {
// Here will be adding the static listener so we can keep the reference
eventListeners.current = { keydown: keyDownHandler, keyup: keyUpHandler };
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Hey, what's the point of useRef vs a local variable in the useEffect callback function?

(e: KeyboardEvent) => e.key == 'Shift' && setInputStep(10),
[],
);
const keyUpHandler = useCallback(
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Why use useCallback instead of setting directly inside the useEffect? Seems like a a simple functionality with extra steps

@almarzn
Copy link
Copy Markdown

almarzn commented Nov 11, 2024

Copy link
Copy Markdown
Owner

@rockfactory rockfactory left a comment

Choose a reason for hiding this comment

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

Hi @aniforprez and thanks for the contribution! About the events setup, I have two notes:

  1. I think the @almarzn suggestion is right here, we can probably get it in an easier way without having to handle refs
  2. In general, I am a bit worried about all the event listeners we're adding (two for every Input).

Since we don't really need any specific functionaly on shift clicking, we could just have one handler (with useHotkeys or with window.addEventListener should be similar, I'd prefer useHotkeys since it's battle tested).
On shift/unshift we can store the value a custom zustand slice (technically it could just be a custom context, but I'd prefer to keep it in zustand to make it easier for DX)

So like an hotkeysSlice with setInputStep.

Then in every FactoryNumberInput we'd just need to do const inputStep = useStore(state => state.hotkeys.inputStep);` and use that as step

What do you think?

@rockfactory rockfactory added the need changes Changes have been requested label Nov 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

need changes Changes have been requested

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants