Support passing IDOMElementDescriptors as targets #1446
Conversation
| @@ -87,7 +87,7 @@ to continue to emulate how actual browsers handle unfocusing a given element. | |||
|
|
|||
| #### Parameters | |||
|
|
|||
| * `target` **([string][64] | [Element][65])** the element or selector to unfocus (optional, default `document.activeElement`) | |||
| * `target` **([string][64] | [Element][65] | IDOMElementDescriptor)** the element, selector, or descriptor to unfocus (optional, default `document.activeElement`) | |||
There was a problem hiding this comment.
this may be nitpicky, but do we have a precedence for prefixing interfaces with I? I know there are eslint rules against against it in @typescript-eslint (as they don't really want to be associated with C# / Java / etc).
(otherwise, can we drop the I? <3 )
There was a problem hiding this comment.
This name is specified in the RFC and implemented in dom-element-descriptors (and now referenced in PRs in several other libraries), so I guess if you want this changed, that needs to be done via an amendment to the RFC?
There was a problem hiding this comment.
ah, it's probably fine then. not worth the effort to change this if folks already agreed to use it
| @@ -32,7 +39,14 @@ function getElement(target: Target): Element | Document | null { | |||
| } else if (target instanceof Window) { | |||
| return target.document; | |||
| } else { | |||
| throw new Error('Must use an element or a selector string'); | |||
There was a problem hiding this comment.
non-blocking, and maybe stylistic, but I really like using assert for type-narrowing and error-communication.
this bit of code would be:
assert('Must use an element, sel...', descriptorData)
return resolveDOMElement(descriptorData);- less nesting
- less branching
- we want an error anyway
✨
| if (typeof target === 'string') { | ||
| let rootElement = getRootElement(); | ||
|
|
||
| return rootElement.querySelectorAll(target); | ||
| } else { | ||
| throw new Error('Must use a selector string'); | ||
| let descriptorData = lookupDescriptorData(target); |
There was a problem hiding this comment.
so here, I suppose
| @@ -100,13 +101,16 @@ export default function doubleClick( | |||
| .then(() => runHooks('doubleClick', 'start', target, _options)) | |||
| .then(() => { | |||
| if (!target) { | |||
| throw new Error('Must pass an element or selector to `doubleClick`.'); | |||
| throw new Error( | |||
There was a problem hiding this comment.
Since this error is used in a good number of places, I think it may be worth using a custom error class, so the message is predictably consistent - free from typos?
| if (isDocument(element)) { | ||
| nodeType = 'Document'; | ||
| } else { | ||
| // This is an error check for non-typescript callers passing in the |
There was a problem hiding this comment.
thanks for adding a "why" comment.
not enough of these in OSS. <3
| @@ -9,6 +9,7 @@ import { | |||
| import { buildInstrumentedElement, insertElement } from '../../helpers/events'; | |||
| import { isEdge } from '../../helpers/browser-detect'; | |||
| import hasEmberVersion from '@ember/test-helpers/has-ember-version'; | |||
| import { createDescriptor } from 'dom-element-descriptors'; | |||
There was a problem hiding this comment.
this library seems useful! it's doing a lot of heavy lifting!
|
Released in 3.3.0 |
Implement support for RFC #726