-
Notifications
You must be signed in to change notification settings - Fork 350
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
Move most of perseus - util.ts
into perseus-core - util.ts
#2089
base: LEMS-2737-2/utils
Are you sure you want to change the base?
Conversation
perseus - util.ts
into perseus-core - util.ts
npm Snapshot: PublishedGood news!! We've packaged up the latest commit from this PR (405428c) and published it to npm. You Example: yarn add @khanacademy/perseus@PR2089 If you are working in Khan Academy's webapp, you can run: ./dev/tools/bump_perseus_version.sh -t PR2089 |
Size Change: +505 B (+0.03%) Total Size: 1.47 MB
ℹ️ View Unchanged
|
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.
Looks good. I'd just suggest moving some UI functions back into Perseus if we can.
unescapeMathMode, | ||
random, | ||
deepClone, | ||
} as const; |
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.
What's your appetite like for switching these to simple named exports instead of bundling them in an object first? We could still export them as a Util
namespace in the top-level index.ts
so the imports don't change...
left: number; | ||
}; | ||
|
||
type TouchHandlers = { |
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.
Having references to UI concepts in perseus-score
raises a warning flag for me. What do you think of leaving anything to do with UI (touch, event handling, anything touching window
).
function parseQueryString(query: string): QueryParams { | ||
// TODO(jangmi, CP-3340): Use withLocation to access SSR safe location. | ||
// eslint-disable-next-line no-restricted-syntax | ||
query = query || window.location.search.substring(1); |
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 think this is safe to move, but we should probably drop the || window...
bit as that won't be safe in a non-browser env.
for (let i = 0; i < len; i++) { | ||
if ( | ||
event.changedTouches[i].identifier === | ||
touchHandlers.currentTouchIdentifier | ||
) { | ||
touchOrEvent = event.changedTouches[i]; | ||
} | ||
} | ||
} else { | ||
touchOrEvent = event; | ||
} | ||
|
||
const isEndish = | ||
event.type === "touchend" || event.type === "touchcancel"; | ||
if (touchOrEvent && isEndish) { | ||
touchHandlers.pointerDown = false; | ||
touchHandlers.currentTouchIdentifier = null; | ||
} | ||
} else { | ||
// touchstart or mousedown | ||
touchHandlers.pointerDown = true; | ||
if (event.changedTouches) { | ||
touchOrEvent = event.changedTouches[0]; | ||
touchHandlers.currentTouchIdentifier = touchOrEvent.identifier; | ||
} else { | ||
touchOrEvent = event; | ||
} | ||
} | ||
|
||
if (touchOrEvent) { | ||
return { | ||
left: touchOrEvent.pageX, | ||
top: touchOrEvent.pageY, | ||
}; | ||
} | ||
} | ||
|
||
// Older browsers don't support passive events and so we need to feature- | ||
// detect them and do event subscription differently for them. | ||
// See: orderer.jsx | ||
const supportsPassiveEvents: () => boolean = () => { | ||
// Test via a getter in the options object to see if the passive | ||
// property is accessed | ||
try { | ||
const opts = Object.defineProperty({}, "passive", { | ||
get: function () { | ||
supportsPassive = true; | ||
}, | ||
}); | ||
// @ts-expect-error - TS2769 - No overload matches this call. | ||
window.addEventListener("testPassive", null, opts); | ||
// @ts-expect-error - TS2769 - No overload matches this call. | ||
window.removeEventListener("testPassive", null, opts); | ||
} catch { | ||
// Intentionally left empty! | ||
} | ||
|
||
return supportsPassive; | ||
}; | ||
|
||
/** | ||
* Gets the word right before where the textarea cursor is | ||
* | ||
* @param {Element} textarea - The textarea DOM element | ||
* @return {JSON} - An object with the word and its starting and ending positions in the textarea | ||
*/ | ||
function getWordBeforeCursor(textarea: HTMLTextAreaElement): WordAndPosition { | ||
const text = textarea.value; | ||
|
||
const endPos = textarea.selectionStart - 1; | ||
const startPos = | ||
Math.max( | ||
text.lastIndexOf("\n", endPos), | ||
text.lastIndexOf(" ", endPos), | ||
) + 1; | ||
|
||
return { | ||
string: text.substring(startPos, endPos + 1), | ||
pos: { | ||
start: startPos, | ||
end: endPos, | ||
}, | ||
}; | ||
} | ||
|
||
/** | ||
* Moves the textarea cursor at the specified position | ||
* | ||
* @param {Element} textarea - The textarea DOM element | ||
* @param {int} pos - The position where the cursor will be moved | ||
*/ | ||
function moveCursor(textarea: HTMLTextAreaElement, pos: number): void { | ||
textarea.selectionStart = pos; | ||
textarea.selectionEnd = pos; | ||
} | ||
|
||
const textarea = { | ||
getWordBeforeCursor, | ||
moveCursor, | ||
} as const; |
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.
This all feels like it should stay in perseus
.
Summary:
I couldn't move the whole of
util.ts
fromperseus
toperseus-core
because of some of its dependencies, but I moved as much as I could. Most of this is just changing imports.Issue: LEMS-2737