-
Notifications
You must be signed in to change notification settings - Fork 194
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
check if element has a size (#50) #51
Conversation
Can you remove the indentation changes? |
Hi @skovhus , just wanted to touch base here. I'm keen to give your PR a proper review and merge but if you could first rebase & remove the whitespace changes it would really help. Thanks! |
I cannot release remove white space as the whole block is indented. Would be really great to refactor that giant function. But I will rebase. ; ) |
Ah ok - yeah we do need to split up this giant function at some point :) I'm thinking of putting together a big major-version roadmap for early 2017, and this would definitely be on the cards.
That's cool, but in that case could you please point out specifically what things you are changing? Just a bit difficult to see in the current diff :) Thanks |
The diff without whitespace (?w=0) makes it fairly clear I think https://github.com/joshwnj/react-visibility-sensor/pull/51/files?w=0 But let me do an updated PR : ) |
Oh that is heaps better, thanks :) I have to remember that |
78c26e6
to
4eb3d60
Compare
I've updated the PR by rebasing with master, but I would like one of you to manually test this change. More unit tests are required here in order to make a change like this safe. |
Hi! Could someone please tell when this will be merged? Or there are some better solution? |
superceded by #114 |
Fixes #50... Sorry that I didn't add any tests for this. Could be nice with some unit tests.
Maybe look at this with whitespace disabled: https://github.com/joshwnj/react-visibility-sensor/pull/51/files?w=0