[EuiColorPicker] onBlur and data-test-subj
#4822
Conversation
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_4822/ |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_4822/ |
| data-test-subj="colorPickerAnchor" | ||
| data-test-subj="test subject string" |
There was a problem hiding this comment.
Did you mean to remove the old one? I'd suspect you'd just want to append any custom DTS? Otherwise I'd consider this a breaking change as there might be consumers targeting the old manual DTS.
There was a problem hiding this comment.
This is more just validation that a DTS passed by a consumer will get used; test subject string should've been here previously but we were ignoring the prop and using the manual default.
We could consider this a breaking change because I did switch to using a generated ID rather than colorPickerAnchor
There was a problem hiding this comment.
Right but should it be overriding or appending like with do with custom classNames?
There was a problem hiding this comment.
Oh good point; we could append. I forgot that findTestSubject uses '~=', // Exists in a space-separated list by default for selector matching. I'll update.
Would you also like to revert the generated ID change to avoid the breaking change?
There was a problem hiding this comment.
I think the manual string is a good approach so that if consumers want to target the EUI supplied one, they can and it doesn't change every render/test run. But the previous one lacked the eui prefix, so I'd still make the breaking change but use euiColorPickerAnchor (or something most appropriate but with the eui prefix).
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_4822/ |
| @@ -283,6 +284,12 @@ export const EuiColorPicker: FunctionComponent<EuiColorPickerProps> = ({ | |||
| onChange(text, output); | |||
| }; | |||
|
|
|||
| const handleOnBlur = () => { | |||
| if (!isColorSelectorShown && onBlur) { | |||
There was a problem hiding this comment.
Curious why the caveat of if !isColorSelectorShown here? Maybe just add a comment for the future?
There was a problem hiding this comment.
Prevents onBlur from being called twice if the popover is open. I'll add a comment 👍
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_4822/ |
Summary
Fixes #4803 and fixes #4792, two bugs related to prop propagation.
onBlurcallback will get called after leaving the main inputdata-test-subjconfiguration is respectedChecklist
- [ ] Check against all themes for compatibility in both light and dark modes- [ ] Checked in mobile- [ ] Props have proper autodocs and playground toggles- [ ] Added documentation- [ ] Checked Code Sandbox works for the any docs examples