Support passing IDOMElementDescriptors as targets#1446
Support passing IDOMElementDescriptors as targets#1446NullVoxPopuli merged 1 commit intoemberjs:masterfrom
Conversation
| #### 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
| } 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
✨
| return rootElement.querySelectorAll(target); | ||
| } else { | ||
| throw new Error('Must use a selector string'); | ||
| let descriptorData = lookupDescriptorData(target); |
| .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
| 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