Conversation
|
Thanks for changing the arrow keys, this feels much better! |
|
@thompsongl Here is a design PR with minor changes to cursors and new stop styles. 👉 PR => thompsongl#8
I also found a couple of issues:
|
Update color stop styles
|
Just checked out latest...
|
snide
left a comment
There was a problem hiding this comment.
Some small functionality comments. I didn't dig too far into the JS. Functionality seems pretty awesome. ⭐️
Cleanup of thumb display
chandlerprall
left a comment
There was a problem hiding this comment.
Didn't find any big issues, very nice work everyone!
I'll give this a shot |
snide
left a comment
There was a problem hiding this comment.
Tested new additions. They work how I'd expect. From a functional / design / accessibility standpoint this LGTM.
ryankeairns
left a comment
There was a problem hiding this comment.
Looks and works great. Just a couple of docs feedback items (one may be a repeat of Chandler's feedback):
- What is the intent of the 'Prop Change' button up top? It wasn't clear to me, but it seems to reset the colors/defaults? If that stays, the wording could be clearer and it may also benefit from looking like a button with a spacer below it.
- The last example would benefit from a label - something like 'Custom example'. Going a step further maybe these should be its own stand along section on this page? ( cc:/ @snide )
|
That z-index hover change is, IMO, much better, thank you! |
Removing this. It was just there for me to ensure prop changes would propagate.
I'm going to rearrange the whole thing, but I did find that the "Display toggles" component doesn't work with form rows |
ryankeairns
left a comment
There was a problem hiding this comment.
Yasssss. Great work on the docs cleanup!
|
Is there a way to make |
Currently, no, but we can work out a way where users would be able to define the bounds. Basically, there would always be two stops (one at each end) that will accept any number. @snide @ryankeairns Is this something we want to add to this PR or do a follow-up? |
|
@thompsongl since maps is the only known consumer for this one, i think we'd need to add that functionality in before we delivered it. You could always merge now if you just want to make the review smaller for the new functionality. |
|
I think a smaller, focused review on the new functionality would be better. I can merge this and follow up. |
|
Adding a stop at the beginning of the track and then using the keyboard focus + enter creates new stops off the track |
😄 I know why that is. I'll fix it before merging this |
chandlerprall
left a comment
There was a problem hiding this comment.
Changes LGTM; pulled and tested locally, only issue found is an edge case in using keyboard navigation to create a new color stop
Sounds great to merge as-is and then add a follow up PR to allow setting min and max. |
* WIP * WIP: scaling clean up * reverse up/down keys * add left/right movement * update test * doubleClick addStop; clean up; tests removal * use more range components directly * Firefox cleanup * i18n; refactor * WIP: add new stop via click * clean up * better keyboard interaction * add stop styles * empty set; backspace fix; drag handle fix * readOnly and disabled * screen reader * more i18n * cleanup of thumbs * Update src/components/color_picker/color_stops/color_stop_thumb.tsx Co-Authored-By: dave.snider@gmail.com <dave.snider@gmail.com> * thumb title * required label * misc feeback * update zIndex for active thumb * docs refactor * util; basic snapshot tests * color stops tests * thumb snapshot tests * CL * use sorted array for add via keyboard location
Summary
Closes #2344 (see full description & background in the issue)
Checklist