-
Notifications
You must be signed in to change notification settings - Fork 712
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
typescript(vx-responsive): re-write package in TypeScript #517
Conversation
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.
Thanks for converting this one! it's perhaps one of our most-used packages 😎 Had a couple comments on it.
@williaster Thanks for the review! Should be good now :) |
}; | ||
|
||
animationFrameID: number | null; | ||
ro: ResizeObserver | undefined; |
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.
do we want to use more verbose name?
const { parentWidth, parentHeight } = this.state; | ||
return ( | ||
<div | ||
style={{ width: '100%', height: '100%' }} |
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.
Convert to constant.
Note: This 100%
style is known to create a layout bug that leave some area of the screen empty when the children use less than 100% width or height, e.g. 50% width & 50% height. This is because this div
always secure 100%
width & height bounding box in the layout.
window.removeEventListener('resize', this.handleResize, false); | ||
} | ||
|
||
resize(/** event */) { |
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.
use arrow function for binding?
if (this.ro) this.ro.disconnect(); | ||
} | ||
|
||
resize({ width, height }: { width: number; height: number }) { |
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.
use arrow function for binding?
looks like we might need to add types for also, updated base branch to master. |
@williaster I bumped |
Hrmm, that's strange 🙈 Do you think it'd be fixable by changing the syntax to something like
instead of
|
@williaster that will work, thanks! Should be fixed now:) |
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.
dope!
🚀 Enhancements
This PR builds off of #488 which added TypeScript build support, and converts the
@vx/responsive
package to TypeScript.Closes #514.
Testing
@williaster @hshoff @schillerk @milesj @kristw