Typescript: Convert story editor utils #12737
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
# Conflicts: # packages/story-editor/src/utils/getSessionStorageKey.ts # packages/story-editor/src/utils/useRefreshPostEditURL.ts # packages/story-editor/tsconfig.json
spacedmonkey
added
Type: Code Quality
|
Plugin builds for 6fd2808 are ready 🛎️!
|
|
Size Change: +121 B (0%) Total Size: 2.71 MB
ℹ️ View Unchanged
|
swissspidy
reviewed
Dec 8, 2022
barklund
approved these changes
Dec 8, 2022
| @@ -23,15 +23,20 @@ import { useRef, useCallback } from '@googleforcreators/react'; | |||
| */ | |||
| import { requestIdleCallback, cancelIdleCallback } from './idleCallback'; | |||
|
|
|||
| interface Task { | |||
| taskId: string | null; | |||
| task: number | null | (() => Promise<void>); | |||
There was a problem hiding this comment.
Why can task be a number? What would that be used for?
There was a problem hiding this comment.
Because it changes from a promise to idleCallbackId, which is an int. That int is then passed to clearQueuedTask which expects an int.
Really hard to follow, I know, took me a while to understand.
| if (activeElement) { | ||
| const { tagName, type, contentEditable } = activeElement as | ||
| | HTMLInputElement | ||
| | HTMLAnchorElement |
There was a problem hiding this comment.
Why anchor elements? contentEditable can be applied to any element.
There was a problem hiding this comment.
Recommended by @miina
There was a problem hiding this comment.
@spacedmonkey The idea was that previously you had created a custom interface to support tagName, type, contentEditable, however, I mentioned there are existing default types already. I did not look into it thoroughly though, so there might be some missing.
@barklund I don't think that contentEditable and type both exists on Element / HTMLElement so this needs more precision. In this particular case, we are trying to detect if native copy-paste should take over or not.
There was a problem hiding this comment.
The typing is hard here. It can be multiple types, that is valid. Not all types have the same props, some have contentEditable and other have type. Not sure what to do here other than what as been done. Maybe a custom type.
swissspidy
reviewed
Dec 9, 2022
swissspidy
reviewed
Dec 9, 2022
swissspidy
reviewed
Dec 9, 2022
Co-authored-by: Morten Barklund <morten.barklund@xwp.co>
swissspidy
reviewed
Dec 9, 2022
swissspidy
approved these changes
Dec 9, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Context
Summary
Covert the simple utils in story editor to use type script.
Relevant Technical Choices
The following files were not converted, for these reasons.
To-do
User-facing changes
Testing Instructions
This PR can be tested by following these steps:
Reviews
Does this PR have a security-related impact?
Does this PR change what data or activity we track or use?
Does this PR have a legal-related impact?
Checklist
Type: XYZlabel to the PRFixes #12740