-
Notifications
You must be signed in to change notification settings - Fork 470
feat: migrate some files to typescript #848
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 7 commits
f215937
61214fc
b71dccd
c3b9704
8c82991
2da06c8
9942434
03f20e1
a2b9e44
d15238b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
@@ -1,3 +1,4 @@ | ||||||||||
import {isNotNull} from './utils' | ||||||||||
import {TEXT_NODE} from './helpers' | ||||||||||
|
||||||||||
const labelledNodeNames = [ | ||||||||||
|
@@ -10,7 +11,9 @@ const labelledNodeNames = [ | |||||||||
'input', | ||||||||||
] | ||||||||||
|
||||||||||
function getTextContent(node) { | ||||||||||
function getTextContent( | ||||||||||
node: Node | Element | HTMLInputElement, | ||||||||||
): string | null { | ||||||||||
if (labelledNodeNames.includes(node.nodeName.toLowerCase())) { | ||||||||||
return '' | ||||||||||
} | ||||||||||
|
@@ -22,37 +25,46 @@ function getTextContent(node) { | |||||||||
.join('') | ||||||||||
} | ||||||||||
|
||||||||||
function getLabelContent(node) { | ||||||||||
function getLabelContent(node: Node | Element | HTMLInputElement) { | ||||||||||
let textContent | ||||||||||
if (node.tagName.toLowerCase() === 'label') { | ||||||||||
if ('tagName' in node && node.tagName.toLowerCase() === 'label') { | ||||||||||
marcosvega91 marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||
textContent = getTextContent(node) | ||||||||||
} else if ('value' in node) { | ||||||||||
textContent = node.value | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||
} else { | ||||||||||
textContent = node.value || node.textContent | ||||||||||
textContent = node.textContent | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could this work?
Suggested change
Or even
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The problem here is that node can't be null but at the same time only input has value. |
||||||||||
} | ||||||||||
return textContent | ||||||||||
} | ||||||||||
|
||||||||||
// Based on https://github.com/eps1lon/dom-accessibility-api/pull/352 | ||||||||||
function getRealLabels(element) { | ||||||||||
if (element.labels !== undefined) return element.labels ?? [] | ||||||||||
function getRealLabels(element: Element | HTMLInputElement) { | ||||||||||
// for old browsers | ||||||||||
// eslint-disable-next-line @typescript-eslint/no-unnecessary-condition | ||||||||||
if ('labels' in element && element.labels !== undefined) | ||||||||||
return element.labels ?? [] | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would prefer we don't change any runtime behavior but rather override the typechecker with type-casting or There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The problem here is the labels can be null and not undefined. Btw I think that on old browser like ie labels is undefined and not null There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks to me covered all these cases and TypeScript is not able to narrow. Let's keep the runtime and annotate the compile time properly. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The problem is that the linter is giving the problem, so There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ignore the linter. |
||||||||||
|
||||||||||
if (!isLabelable(element)) return [] | ||||||||||
|
||||||||||
const labels = element.ownerDocument.querySelectorAll('label') | ||||||||||
return Array.from(labels).filter(label => label.control === element) | ||||||||||
} | ||||||||||
|
||||||||||
function isLabelable(element) { | ||||||||||
function isLabelable(element: Element) { | ||||||||||
const labelableRegex = /BUTTON|METER|OUTPUT|PROGRESS|SELECT|TEXTAREA/ | ||||||||||
return ( | ||||||||||
element.tagName.match(/BUTTON|METER|OUTPUT|PROGRESS|SELECT|TEXTAREA/) || | ||||||||||
labelableRegex.test(element.tagName) || | ||||||||||
(element.tagName === 'INPUT' && element.getAttribute('type') !== 'hidden') | ||||||||||
) | ||||||||||
} | ||||||||||
|
||||||||||
function getLabels(container, element, {selector = '*'} = {}) { | ||||||||||
const labelsId = element.getAttribute('aria-labelledby') | ||||||||||
? element.getAttribute('aria-labelledby').split(' ') | ||||||||||
: [] | ||||||||||
function getLabels( | ||||||||||
container: Element, | ||||||||||
element: Element, | ||||||||||
{selector = '*'} = {}, | ||||||||||
) { | ||||||||||
const ariaLabelledBy = element.getAttribute('aria-labelledby') | ||||||||||
const labelsId = isNotNull(ariaLabelledBy) ? ariaLabelledBy.split(' ') : [] | ||||||||||
marcosvega91 marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||
return labelsId.length | ||||||||||
? labelsId.map(labelId => { | ||||||||||
const labellingElement = container.querySelector(`[id="${labelId}"]`) | ||||||||||
|
@@ -67,7 +79,6 @@ function getLabels(container, element, {selector = '*'} = {}) { | |||||||||
const labelledFormControl = Array.from( | ||||||||||
label.querySelectorAll(formControlSelector), | ||||||||||
).filter(formControlElement => formControlElement.matches(selector))[0] | ||||||||||
|
||||||||||
return {content: textToMatch, formControl: labelledFormControl} | ||||||||||
}) | ||||||||||
} | ||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
export * from '../matches' | ||
export * from '../matches.ts' | ||
export * from '../get-node-text' | ||
export * from '../query-helpers' | ||
export * from '../config' | ||
export * from '../config.ts' |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
export function isNotNull<T>(arg: T): arg is NonNullable<T> { | ||
return arg !== null | ||
} | ||
marcosvega91 marked this conversation as resolved.
Show resolved
Hide resolved
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting. Is the extension required? I didn't think it was and would prefer to not have to specify it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like too, but I didn't find a way to not specify it.
eslint
gave me some trouble.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can deal with that later. Not a ship stopper.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll leave the point open, maybe someone could help