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

Remove style and className, and monitor children instead #13

Merged
merged 1 commit into from
Jun 16, 2015

Conversation

gaearon
Copy link
Contributor

@gaearon gaearon commented Jun 15, 2015

This addresses #12.

Instead of having the component always render its own div, monitor whatever passed to its children (and use a single <span> as the default child):

// No magic <div>s now, all markup is controlled by you!
<VisibilityMonitor>
  <div className='whatever' />
</VisibilityMonitor>

// The old way still works though: by default, it renders <span>
<VisibilityMonitor />

This changes the public API:

  • removes className
  • removes style
  • allows to pass a single child for monitoring

I also took the liberty to rename component to element in tests because that matches the current React terminology. I'm happy to roll that back if you don't like it.

@gaearon
Copy link
Contributor Author

gaearon commented Jun 15, 2015

Note: I couldn't figure out how to build the example (I keep getting build errors) so I didn't check if it works. Tests are passing.

@joshwnj
Copy link
Owner

joshwnj commented Jun 16, 2015

@gaearon thanks! Didn't know about React.Children.only until now :)

I'm happy with those naming changes as well, I agree there's value in consistency with React terminology.

Btw could you please open a new issue with build errors from the example?

joshwnj added a commit that referenced this pull request Jun 16, 2015
Remove style and className, and monitor children instead
@joshwnj joshwnj merged commit 66ce5ef into joshwnj:master Jun 16, 2015
@joshwnj
Copy link
Owner

joshwnj commented Jun 17, 2015

sorry about that @kof ! I'll be more careful with the versioning next time.

I've created a v1.x branch and republished v1.8.0 which is pre-breaking-change, so please upgrade to that. I'll continue to maintain v1.x for now with bugfixes, but I'd like to figure out if there's a way to converge. I'll create a new issue to discuss that and get ideas.

@gaearon v2.0.0 has the new changes from this PR.

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.

3 participants