Feat: Allow to define the default width/height #8
Conversation
|
Yeah, makes sense. 👍 |
|
Actually, one more thing: could you try adding a test for it? |
|
Hey, I was going to add a test initially, but the current test structure is hard to extend. So I didn't do it. |
|
Yeah I'll have to do it after the my wedding, about a month from now.
Just nudge me then again.
…On Tue, 21 May 2019 at 03:43, Simon Boudrias ***@***.***> wrote:
Hey, I was going to add a test initially, but the current test structure
is hard to extend. So I didn't do it.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#8?email_source=notifications&email_token=AA4CKEVR57S2M3DL43FRNHDPWNHSPA5CNFSM4HN6EBH2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODV2QGWQ#issuecomment-494207834>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AA4CKEUZCGBYHEJJAUVZRVLPWNHSPANCNFSM4HN6EBHQ>
.
|
|
I've updated the CHANGELOG and updated the version. Hopefully we can move forward without the tests as it's not really bringing a lot of values in this specific case. |
| @@ -1,10 +1,10 @@ | |||
| import { useEffect, useState, useRef } from "react"; | |||
| import ResizeObserver from "resize-observer-polyfill"; | |||
|
|
|||
| export default function() { | |||
| export default function(defaultWidth = 1, defaultHeight = 1) { | |||
There was a problem hiding this comment.
I'd suggest defaulting the default (😆) to 0 so that we can use !!width to test if we've got a width.
There was a problem hiding this comment.
It could be that width / height is actually zero though.
Could return nulls, but I'm not sure it's worth the effort, and it's a breaking change as well.
Do you have a good use-case you'd need this for? @FokkeZB
There was a problem hiding this comment.
Ah, yes... then only null would work. I don't have a direct need for it right now, but I do think it would make sense to tell the default from an actual value.
There was a problem hiding this comment.
Feel free to create a separate issue when you have a use-case for it and then we can discuss it. 👍
|
I've moved things to #10 , which includes tests as well. Also, could you advise on how to update the TS types? |
|
I think I'll just remove types and do a major release. |
For use within SSR context, it's quite useful to be able to define a default size.
Let me know if this change makes sense for you.