kind of type math-input #415
Conversation
🦋 Changeset detectedLatest commit: d7ce807 The changes in this PR will be included in the next version bump. This PR includes changesets to release 0 packagesWhen changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
GeraldRequired Reviewers
Don't want to be involved in this pull request? Comment |
|
Size Change: +233 B (0%) Total Size: 635 kB
ℹ️ View Unchanged
|
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #415 +/- ##
==========================================
+ Coverage 65.49% 65.97% +0.48%
==========================================
Files 481 487 +6
Lines 103927 107131 +3204
Branches 5620 7376 +1756
==========================================
+ Hits 68064 70685 +2621
- Misses 35863 36446 +583
... and 38 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
| @@ -239,15 +285,15 @@ class MathInput extends React.Component { | |||
| }; | |||
|
|
|||
| /** Gets and cache they bounds of the keypadElement */ | |||
| _getKeypadBounds = () => { | |||
| _getKeypadBounds: () => $FlowFixMe = () => { | |||
There was a problem hiding this comment.
I think all these things dealing with bounds originate from a call to getBoundingClientRect() which is typed as returning a DOMRect. Could we use that in this file?
There was a problem hiding this comment.
I couldn't get this to work and I don't want to linger too long on this. I was able to add _keypadBounds: ?ClientRect; but when I used ClientRect or ?ClientRect as the return value here errors started to spread.
I'm sure it's fixable, but I think I would need to add logic/checks which I'm trying to avoid in this PR.
| @@ -651,7 +630,7 @@ class MathInput extends React.Component<Props, State> { | |||
| } | |||
| }; | |||
|
|
|||
| handleTouchEnd: ($FlowFixMe) => void = (e) => { | |||
| handleTouchEnd: (SyntheticTouchEvent<HTMLDivElement>) => void = (e) => { | |||
There was a problem hiding this comment.
The other touch handles use HTMLSpanElement. Should these be the same? If so, I wonder if we should declare a type alias at the top of the file that represents the element type. Ideally I'd love to type this as the result of the render() call, but I don't think it's easy/possible to "extract" that type using Flow utility types.
At a min let's just make sure we have the right DOM element type across all the event registrations.
Given that we don't refer to the target in many of these, I wonder if we can just do SyntheticTouchEvent<*>.. 🤔
There was a problem hiding this comment.
There are two different inputs that are triggering touch events:
- The
Viewis the container for the math-input and is adiv - The
CursorHandleis another interactive element and is aspan
EDIT: when I tried to type the handlers on the window object I found this and decided it wasn't worth the time.
Summary:
Just a very quick pass to get some Flow typing in
math-input. I would happily continue, but I'm also supposed to be shipping updates to users too.Issue: LC-618
Test plan: