typings: withSize add onSize callback #167
Conversation
Codecov Report
@@ Coverage Diff @@
## master #167 +/- ##
=======================================
Coverage 90.56% 90.56%
=======================================
Files 4 4
Lines 106 106
Branches 25 25
=======================================
Hits 96 96
Misses 9 9
Partials 1 1Continue to review full report at Codecov.
|
|
@ctrlplusb can you review? |
react-sizeme.d.ts
Outdated
| refreshRate?: number | ||
| refreshMode?: 'throttle' | 'debounce' | ||
| noPlaceholder?: boolean | ||
| readonly monitorWidth?: boolean |
There was a problem hiding this comment.
Not sure if there is any value marking these as read-only as they won't get exposed to the user. These configuration values are inputs.
react-sizeme.d.ts
Outdated
| size: { | ||
| width: number | null | ||
| height: number | null | ||
| readonly size: { |
react-sizeme.d.ts
Outdated
| } | ||
|
|
||
| export interface SizeMeRenderProps extends SizeMeOptions { | ||
| children: (props: SizeMeProps) => ReactElement | ||
| readonly children: (props: SizeMeProps) => ReactElement |
There was a problem hiding this comment.
Same here, this is user provided, and won't get exposed in a manner that the user could modify it, so the readonly isn't really useful in this case.
react-sizeme.d.ts
Outdated
| } | ||
|
|
||
| export class SizeMe extends Component<SizeMeRenderProps> {} | ||
|
|
||
| export type WithSizeOnSizeArgs = [ |
There was a problem hiding this comment.
Could we rename this to Size and perhaps reuse the type within SizeMeProps?
react-sizeme.d.ts
Outdated
| } | ||
| ] | ||
|
|
||
| export type WithSizeOnSizeCallback = (...x: WithSizeOnSizeArgs) => void |
There was a problem hiding this comment.
You are spreading an array/tuple that contains a single item. Perhaps consider simplifying this (after the Size type refactor I recommended above) to the following:
export type OnSizeCallback = (size: Size) => void;
react-sizeme.d.ts
Outdated
| export type WithSizeOnSizeCallback = (...x: WithSizeOnSizeArgs) => void | ||
|
|
||
| export interface WithSizeProps { | ||
| readonly onSize?: WithSizeOnSizeCallback |
There was a problem hiding this comment.
Same here regarding the readonly not really being useful as it's an input that doesn't get exposed to user again.
src/__tests__/typescript/hoc.tsx
Outdated
| @@ -22,4 +22,8 @@ function MyComponent({ id, size }: MyComponentProps) { | |||
|
|
|||
| const SizedMyComponent = withSize()(MyComponent) | |||
|
|
|||
| const foo = <SizedMyComponent id={1} /> | |||
| const onSize: WithSizeOnSizeCallback = ({ height, width }) => { | |||
| Boolean(height && width) | |||
There was a problem hiding this comment.
Could you replace the body of this with the following:
if (width) {
const foo = width + 1 const foo = width + 1
} }
if (height) { if (height) {
const foo = height + 1 const foo = height + 1
} }
// typings:expect-error // typings:expect-error
const h1 = height + 1 const h1 = height + 1
// typings:expect-error // typings:expect-error
const w1 = width + 1 const w1 = width + 1This helps guard that width/height are nullable numbers.
|
@beckend reviewed :) |
Codecov Report
@@ Coverage Diff @@
## master #167 +/- ##
=======================================
Coverage 90.56% 90.56%
=======================================
Files 4 4
Lines 106 106
Branches 25 25
=======================================
Hits 96 96
Misses 9 9
Partials 1 1Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #167 +/- ##
=======================================
Coverage 90.56% 90.56%
=======================================
Files 4 4
Lines 106 106
Branches 25 25
=======================================
Hits 96 96
Misses 9 9
Partials 1 1Continue to review full report at Codecov.
|
|
@ctrlplusb sorry for the delay, life and stuff... |
|
The PR was done, however no action was taken. |
|
Hey @beckend I still intend to merge this. I just need to do some more planning around it. Please leave open. Thanks |
|
Got it, just wanted to do some housecleaning. |
No description provided.