Skip to content
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

Rewrite #72

Merged
merged 15 commits into from
Aug 13, 2016
Merged

Rewrite #72

merged 15 commits into from
Aug 13, 2016

Conversation

nkbt
Copy link
Owner

@nkbt nkbt commented Aug 11, 2016

Well... hmmm... full rewrite

@codecov-io
Copy link

codecov-io commented Aug 11, 2016

Current coverage is 23.52% (diff: 19.14%)

Merging #72 into master will increase coverage by 11.93%

@@             master        #72   diff @@
==========================================
  Files             1          1          
  Lines            69         51    -18   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits              8         12     +4   
+ Misses           61         39    -22   
  Partials          0          0          

Powered by Codecov. Last update c52888b...7ae3da4

const to = this.content.clientHeight;
this.setState({action: STABLE, from: to, to});
}
this.props.onRest();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be better to call the callbacks without providing this context of this.props object, i.e.

const { onRest } = this.props;
onRest();  // no forced `this`

or

(0, this.props.onRest)();  // "global eval"

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why so?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because the callbacks are not methods of this.props object. I generally avoid calling such functions as methods because this changes semantics of what is expected inside these functions (I expect strictly no this inside).

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, thanks

@nkbt
Copy link
Owner Author

nkbt commented Aug 13, 2016

Good idea, done. Also renamed to be more reasonable.

@nkbt
Copy link
Owner Author

nkbt commented Aug 13, 2016

Yay! Looks like all edge-cases from issues are covered now by examples!

@nkbt
Copy link
Owner Author

nkbt commented Aug 13, 2016

Since keepCollapsedContent is removed as well as fixedHeight, I need to add some examples or maybe couple of extra components that will wrap Collapse and implement same functionality

@nkbt
Copy link
Owner Author

nkbt commented Aug 13, 2016

I switched default branch to 2.x so README reflects currently published version and I can merge this to master without consequences.

@nkbt nkbt merged commit 7bd789c into master Aug 13, 2016
@nkbt nkbt deleted the rewrite branch August 13, 2016 09:36
@nkbt nkbt removed the in progress label Aug 13, 2016
@nkbt
Copy link
Owner Author

nkbt commented Aug 13, 2016

Published as react-collapse@3.0.0 or react-collapse@next. Not the main channel yet until better tested in the wild

@sompylasar
Copy link

In my current projects I have no need for Collapse yet, so I won't be able to test it "in the wild". I've reported to my teammates who needed it, maybe they will try it.

@sompylasar
Copy link

sompylasar commented Aug 13, 2016

@nkbt Please could you add a brief migration guide from 2.x to 3.x to the readme? thanks!

@sompylasar
Copy link

@nkbt And the GIF in the readme looks like it's from the old implementation, as it mentions fixedHeight. Needs an update, too.

@sompylasar
Copy link

The Hooks example scrolls to the bottom of the page on page load when opening this page: http://nkbt.github.io/react-collapse/example/

@sompylasar
Copy link

Auto-scroll does not work properly:

0 paragraphs

auto-scroll-hook-empty


3 paragraphs

auto-scroll-hook

@nkbt
Copy link
Owner Author

nkbt commented Aug 13, 2016

Yeah I definitely will add a migration guide. Scrolling is no longer in
hooks example, it is outdated. New examples do not have this
On Sun, 14 Aug 2016 at 05:45, John Babak notifications@github.com wrote:

Auto-scroll does not work properly:
0 paragraphs

[image: auto-scroll-hook-empty]

https://cloud.githubusercontent.com/assets/498274/17645206/7f8682c2-61a7-11e6-8da1-bba17f0bc481.gif

3 paragraphs

[image: auto-scroll-hook]
https://cloud.githubusercontent.com/assets/498274/17645208/83a3caae-61a7-11e6-929b-4e01298b15be.gif


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#72 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAKsoLUUFxYPNS3F27fcsiLS_Unig0pLks5qfh7ggaJpZM4JiA0O
.

@sompylasar
Copy link

Yeah I definitely will add a migration guide.

Thanks!

Scrolling is no longer in hooks example, it is outdated. New examples do not have this

I for some reason thought that nkbt,github.io is linked to the new branch. OK.

@nkbt
Copy link
Owner Author

nkbt commented Aug 14, 2016

Not yet, until normal release. https://npmcdn.com/react-collapse@3.0.0/build/react-collapse.js -- I need to make a codepen with it for now (or maybe several)

@nkbt
Copy link
Owner Author

nkbt commented Aug 14, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants