Open bengfarrell opened 3 years ago
I really like this and would love to work out the right way to bring this into the library. Currently, we leverage a version of this in the Color elements. There we actually allow multiple modifier keys to multiply the multiplier: https://github.com/adobe/spectrum-web-components/blob/main/packages/color-area/src/ColorArea.ts#L212
Maybe that's overkill? Either way would that be a nice feature to abstract in a reusable manner across elements including the Number Field, standard Slider and Color Elements broadly?
While I'm thinking in this area, I also wonder if Home
and End
should move to the bounds of min
/max
while available? Something to consider separately, probably.
Could I convince you to submit a PR with this multiplier functionality? I think regardless of merging the nuances between this and other elements that we can likely adopt your approach here ASAP, even if it might not be 100% "Spectrum". Then we can refine its implementation and how it fits into the larger library over time.
This is functionality that we are 100% planning to bring into the library, and I'd be quite happy to start doing so with a PR here as well: separate, preferably.
It's not immediately clear to me from reading the code how it works. Particularly, I see the scrub
functionality stemming from the same listener as the stepper
UI, which I will have to spend some more time with to fully understand. However, if it's working for you, I'd be quite happy to leverage this as the basis for the same in the main library.
One nuance question that I'll have to dig into the Spectrum docs for is whether you can scrub
the input, or just the buttons
. That may be overly restricting, in that the buttons are optional, but shouldn't be a big deal to move around as needed.
Another is you choice to hide scrubbing behind scrubbable
. I'd love a little more information as to why you saw this as a requirement for this feature. It feels like if you can scrub somewhere, you'd expect to be able to do so everywhere, as a user.
Thanks so much for starting the conversation here! Let me know what you think about submitting PRs. I'm not completely averse to just stealing code, but definitely prefer to ensure attribution is made to the right person when possible.
Sounds good! I'll submit both separately tomorrow.
So, in regards to the modified step, I'm definitely not against having multiple modifier keys active, but I really question the results here (in regards to my project anyway). It looks just like for every modifier key held down you run the delta calcs once more over. A potential 4x increase in speed but only if you're jamming on every possible modifier key at once. It doesn't seem the most configurable either. In our designs we're mainly using this for transform positions. So moving something by 4px vs 1px doesn't feel very impactful (and our designer wants 10px when shift is held).
TBH, I was even afraid to even use the shift key because in the original code you've blocked all input if any one of those modifiers are down. Now that I think the floodgates are open, I think I'll change it to any modifier to pair with the customized attribute to change the scale.
I am a bit confused by your "home" and "end" remarks. Just not sure what you're referring to.
On scrubbing, yes - the pointer events took me a while to understand as well. But it seems to be a way to route all pointer events from a directive through to a change function, and the author who wrote it captures the change needed for the specific button as a function, and calls it at specific timed intervals. This is so a user can hold down the button and it keeps running the increment or decrement, but also is careful not to flood the system by calling it too often.
I was a bit uncertain what to do here, because it seemed silly to avoid the existing pointer event handlers and create my own, but obviously I don't want my handlers to repeat like they did either. So I loosely fit my solution to the existing, and was careful to make sure they don't go the same way.
In terms of only being able to scrub the buttons, I know in my app, we plan to hide the buttons all around. I'm not really seeing stepper buttons like these in any modern designs on my end. So if you decide to go down this route, I'd suggest you get in touch with a few designers working on the "you know what" teams. I think this would definitely be a point where my team would diverge from SWC if this decision was made.
Lastly, the scrubbable attribute. I'll happily remove it. I thought I was treading a bit on thin ice even suggesting this feature, which is why I put it behind a flag. I still question how good this will be with managing focus, since it is a bit different (no focus when scrubbing), so it breaks some of SWC's existing rules AFAIK about pointerdown on a component immediately receiving focus. So that was my other reason for putting it behind a flag.
I gated those keys because at the specific implementation thereof it was less work to do that then to add the nuance of handling the specifics of when a modifier outlines that an arrow key move the carat to the beginning of the input as opposed to alters the value. I'll review more closely whether aria documentation prevents certain keys here from participating in this work, but I'd say you're relatively go to go. And, once we have an implementation that works we can edit it for any edge cases later.
Great points ass around. I just resurrected the next
branch to catch "more complex changes" when they aren't fully baked. I'd love to get your implementation into something like that and then work from there, rather than completely make something from scratch. Once we have that we can discuss some easier changes in PR and some larger changes can be queued up from next
before we fully adopt the feature.
Very excited to see these make their way into the library!
For those interested, the following preview sites are up:
Thanks so much for sharing your code on this @bengfarrell!
Great work on these very much requested features! 💯
Regarding the scrubbing
functionality, our designers proposed something similar to this, where the input
itself is left alone and the trigger for scrubbing
to be the field's label
:
https://user-images.githubusercontent.com/8905546/123397316-f4042100-d5aa-11eb-988d-0ee1486cae90.mov
I'm not really sure how this should work with inputs that have no label or some other corner cases, but I thought it's worth sharing this point of view and debate around it if valid.
Great work on these very much requested features! 💯
Regarding the
scrubbing
functionality, our designers proposed something similar to this, where theinput
itself is left alone and the trigger forscrubbing
to be the field'slabel
: https://user-images.githubusercontent.com/8905546/123397316-f4042100-d5aa-11eb-988d-0ee1486cae90.movI'm not really sure how this should work with inputs that have no label or some other corner cases, but I thought it's worth sharing this point of view and debate around it if valid.
That's an interesting idea! Ultimately, I don't feel like this is the right way to go not only for the reason that it may not have a label, but also the label might be on top or on the side and kind of an inconsistent experience in that regard. It probably also wouldn't be very discoverable as a feature on the label. I don't know that a user would ever think to themselves "Let me hover over this label and see if it's interactive", while the very action of using an input field means you have to hover over and click into it. On the way there, you'll see the new cursor.
Honestly, though, doing it other ways didn't cross my mind. Not that it appears to be there anymore (hope it comes back!), but my team really liked the functionality of the number field in XD and Illustrator, so I tried to model the interaction patterns after that because it worked.
thanks for the feedback! Happy to have my mind changed, of course!
needs a11y and Spectrum approval before we can ship this.
Expected Behaviour
So, I could make a PR for this, but I think I'm too far ahead of the curve to even be under consideration against Spectrum design recommendations. I'm also messing with component and input element focus in ways that probably need more thought (that might impact FocusableBase maybe). It's also up to opinion whether the properties I created should act and are named the way I did them.
However, given that I thought more projects might want the feature, I thought I'd offer up a first pass, including storybook documentation as well.
For me, I'm just going to copy and paste into my project regardless, but maybe this'll help!
https://github.com/bengfarrell/spectrum-web-components