Combine the useMove frequently appears with useKeyboard #2508

majornista · 2021-10-28T21:47:05Z

Reopening: #2491 by @snowystinger after it was mistakenly merged into #2483

Closes
Pulled into its own PR for discussion.
useMove frequently appears with useKeyboard to handle page size movements, page movements are still movements, so this refactors to include that information. This has a side benefit that it means people can't mess up the order of keyboard events + use move events which resulted in bad onChange/onChangeEnd values.

Update:
keep pageup/down and home/end separate from useMove to keep useMove from being a one-stop-shop. especially since those keys can mean vastly different things depending on what useMove is being used in. For example, 1d vs 2d movements. People may also want to hook up "wasd" for movement, which isn't covered by useMove and we probably wouldn't add it.
usePress sets precedent for exposing modifier keys, so we're now exposing those from useMove as well. This reduces some complexity for figuring out which keys should be handled outside of useMove

✅ Pull Request Checklist:

Included link to corresponding React Spectrum GitHub Issue.
Added/updated unit tests and storybook for this change (for new code or code which already has tests).
Filled out test instructions.
Updated documentation (if it already exists for this component).
Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

🧢 Your Project:

# Conflicts: # packages/@react-aria/color/src/useColorArea.ts # packages/@react-aria/interactions/src/useMove.ts

majornista · 2021-11-01T13:43:33Z

packages/@react-aria/interactions/src/useMove.ts

+          // TODO: include page size as an option? these are page movements, also Shift + arrows count for that
+          // it can be different depending on the slider/s 2D sliders home/end&pageup/down are on two axis
+          // in 1D sliders, home/end go to min/max, vs pageup/down are page size increments
+          // should triggerKeyboardMove include an is PageMove? include a key?


https://github.com/adobe/react-spectrum/pull/2491/files/13132ed1d7eb3ca48af4d3a78467b8defb1537e9#r736024259

@LFDanLu LFDanLu 7 days ago Member
Related to my useColorWheelState sentiment, IMO adding page size as an option would be nice, but perhaps we can wait till people request it. IMO I think we are fine w/ omitting pageMove/the keyboard key from triggerKeyboardMove for now, did you have any use cases in mind?

@snowystinger snowystinger 7 days ago Author Member
yeah, given that it's really stately that cares about the size, I've been keeping it over there instead of here or in the aria hooks where useMove is used

majornista · 2021-11-01T13:46:26Z

packages/@react-stately/color/src/useColorWheelState.ts

-    increment(minStepSize: number = 0) {
-      let newValue = hue + Math.max(minStepSize, step);
+    increment(isPage: boolean) {
+      let newValue = hue + Math.max(isPage ? PAGE_MIN_STEP_SIZE : 0, step);


https://github.com/adobe/react-spectrum/pull/2491/files/13132ed1d7eb3ca48af4d3a78467b8defb1537e9#r736015385

@LFDanLu LFDanLu 7 days ago Member
Do we not want to offer a customizable minStepSize anymore for increment and decrement? I know previously that useColorWheel set the minStepSize to 6 (hence the 6 being replicated here via PAGE_MIN_STEP_SIZE), but is this a hard standard? Dunno if people would want to modify that value tbh.

Perhaps we can run with this and if people request it we can add minStepSize back?

@snowystinger snowystinger 7 days ago Author Member
not going to add a customization for minStepSize right now

@majornista majornista 6 days ago Member
Should we use snapValueToStep here?

@snowystinger snowystinger 6 days ago Author Member
probably, but this keeps the changes a bit more minimal, happy to improve on it

adobe-bot · 2021-11-01T22:11:56Z

Build successful! 🎉

packages/@react-aria/color/src/useColorArea.ts

devongovett · 2021-11-02T15:42:12Z

packages/@react-aria/color/src/useColorArea.ts

+        } else if (deltaY > 0) {
+          stateRef.current.decrementY(shiftKey);
+        } else if (deltaY < 0) {
+          stateRef.current.incrementY(shiftKey);
        }


So I take it this couldn't be unified with the mouse handling below?

not without adjusting the math in setColorFromPoint, I haven't been able to figure it out yet, i think it would be good followup
the problem lying in the fact that after each keypress, it renders and the so the delta is always 1, so it gets snapped to where it already is. the setColorFromPoint would need to be aware that it's a keyboard event

or, new thought as I'm writing this, we'd need to do some math in useColorArea to change the delta to the channel step size possibly, I'll give it a go

devongovett · 2021-11-02T15:43:07Z

packages/@react-stately/color/src/useColorWheelState.ts

  /** Decrements the hue by the given amount (defaults to 1). */
-  decrement(minStepSize?: number): void,
+  decrement(isPage?: boolean): void,


Isn't this a breaking change?

yeah, though it's beta, so if we wanted to do this, now would be the time
I think this makes more sense, aria hooks don't care what the step sizes are, but the state very much cares

hmm, I don't particularly like passing booleans as parameters though. it's not very extensible for future use cases, and doesn't allow users of stately to override the page size. If we want to keep it in stately, I would make a separate method (e.g. incrementStep - we have incrementPage in datepicker, but in this case they aren't really "pages"). But I also think it would be fine and more flexible to just pass the step size in.

good point, i'll see what we can do about it

adobe-bot · 2021-11-02T21:02:15Z

Build successful! 🎉

adobe-bot · 2021-11-05T01:40:37Z

Build successful! 🎉

devongovett · 2021-11-05T21:49:41Z

packages/@react-stately/color/src/useColorAreaState.ts

@@ -135,11 +137,15 @@ export function useColorAreaState(props: ColorAreaProps): ColorAreaState {
    valueRef.current = color.withChannelValue(channels.yChannel, v);
    setColor(valueRef.current);
  };
+  let xChannelPageStep = Math.max(color.getChannelRange(channels.xChannel).pageSize, xChannelStep);
+  let yChannelPageStep = Math.max(color.getChannelRange(channels.yChannel).pageSize, yChannelStep);


nit: can we avoid using the term "page" for this? It doesn't really make sense. There are no pages 😄

yeah, happy to, got a better name?
bigStep?
leap?

devongovett

lgtm aside from the one nit above. feel free to address that separately though

snowystinger · 2021-11-08T22:15:12Z

Going to merge into color-area, we can keep talking about the name replacement for page there

LFDanLu

LGTM, tested the ColorArea and ColorSlider and things seem to work

* Combine the useMove frequently appears with useKeyboard * fix merge mistake * Remove isPage from useMove, add modifier keys * continue propagation for keys we don't handle * have aria pass through the step size Co-authored-by: Rob Snow <[email protected]>

@trivial

* Initial ColorArea implementation * refactor aria back to stately * more refactoring * refactor step logic and defaults * tests and subsequent fixes from tests * Add uncontrolled to tests * Add size story * use snaptovalue and add page steps * remove dead code * rename variable to make more sense, fix description * Add isDisabled based on Spectrum designs * add min size * add prop description * Get back to no useMove changes * Combine the useMove frequently appears with useKeyboard (#2508) * Combine the useMove frequently appears with useKeyboard * fix merge mistake * Remove isPage from useMove, add modifier keys * continue propagation for keys we don't handle * have aria pass through the step size Co-authored-by: Rob Snow <[email protected]> * ColorArea: remove aria-roledescription on iOS/Android (#2547) * ColorArea: remove aria-roledescription on iOS/Android To better reflect behavior of slider inputs with mobile screen readers on iOS and Android, each input should be labeled by its individual channel name and we should remove aria-roledescription so the input is identified simply as a slider control. * ColorArea: change aria-valuetext for iOS/Android On iOS/Android, the aria-valuetext for each slider within the ColorArea control should just be the value for that channel, and need not include the value for the second channel. The value for all three channels is included as the title attribute. * ColorArea: include channel name in aria-valuetext for iOS/Android * Add formatting for color name and value based on locale * allow for differences in formatting strings * fix lint * @trivial remove extra whitespace/redundant afterEach * fix: tests * fix remaining issues before merges on main make it in * incorporate michaels a11y and locale fixes Co-authored-by: Michael Jordan <[email protected]> Co-authored-by: Daniel Lu <[email protected]> Co-authored-by: GitHub <[email protected]>

snowystinger added 3 commits October 23, 2021 15:33

Combine the useMove frequently appears with useKeyboard

3930da3

Merge branch 'color-area' into refactor-useMove

278fe31

# Conflicts: # packages/@react-aria/color/src/useColorArea.ts # packages/@react-aria/interactions/src/useMove.ts

fix merge mistake

13132ed

majornista requested review from dannify, devongovett and snowystinger as code owners October 28, 2021 21:47

majornista commented Nov 1, 2021

View reviewed changes

majornista requested a review from LFDanLu November 1, 2021 13:47

Remove isPage from useMove, add modifier keys

a4cdb20

devongovett requested changes Nov 2, 2021

View reviewed changes

continue propagation for keys we don't handle

30222da

have aria pass through the step size

98acc21

devongovett reviewed Nov 5, 2021

View reviewed changes

devongovett approved these changes Nov 5, 2021

View reviewed changes

snowystinger merged commit 7d49f0f into color-area Nov 8, 2021

snowystinger deleted the refactor-useMove branch November 8, 2021 22:15

LFDanLu approved these changes Nov 8, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Combine the useMove frequently appears with useKeyboard #2508

Combine the useMove frequently appears with useKeyboard #2508

majornista commented Oct 28, 2021 •

edited by snowystinger

Loading

majornista Nov 1, 2021 •

edited

Loading

majornista Nov 1, 2021

adobe-bot commented Nov 1, 2021

devongovett Nov 2, 2021

snowystinger Nov 2, 2021 •

edited

Loading

devongovett Nov 2, 2021

snowystinger Nov 2, 2021

devongovett Nov 3, 2021

snowystinger Nov 3, 2021

adobe-bot commented Nov 2, 2021

adobe-bot commented Nov 5, 2021

devongovett Nov 5, 2021

snowystinger Nov 5, 2021

devongovett left a comment

snowystinger commented Nov 8, 2021

LFDanLu left a comment

Combine the useMove frequently appears with useKeyboard #2508

Combine the useMove frequently appears with useKeyboard #2508

Conversation

majornista commented Oct 28, 2021 • edited by snowystinger Loading

✅ Pull Request Checklist:

📝 Test Instructions:

🧢 Your Project:

majornista Nov 1, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adobe-bot commented Nov 1, 2021

Choose a reason for hiding this comment

snowystinger Nov 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adobe-bot commented Nov 2, 2021

adobe-bot commented Nov 5, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devongovett left a comment

Choose a reason for hiding this comment

snowystinger commented Nov 8, 2021

LFDanLu left a comment

Choose a reason for hiding this comment

majornista commented Oct 28, 2021 •

edited by snowystinger

Loading

majornista Nov 1, 2021 •

edited

Loading

snowystinger Nov 2, 2021 •

edited

Loading