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

the cb is missed in this._setWrapperHeight() so the current cb logic …(foundation.orbit.js) #10160

Closed
wants to merge 1 commit into from

Conversation

simongcc
Copy link

@simongcc simongcc commented Jun 9, 2017

Sorry for previous failed attempts, finally I understand how to PR it right.
Since it was already a while ago, it is related to my previous opened PR #8755
The callback logic is being added but the callback option and parameter is missed so I am here to add it back.

@kball
Copy link
Contributor

kball commented Jun 9, 2017

Looking at this, I think I understand what you're trying to accomplish, but would actually rather we fire an event so that folks using an indirect instantiation (e.g. using data attributes in the html) can still set up a listener. Can you replace the callback option with a new event being fired at the same point in the lifecycle?

@DanielRuf
Copy link
Contributor

Can you replace the callback option with a new event being fired at the same point in the lifecycle?

Definitely a good idea.

@kball how should we name the event?

Copy link
Contributor

@DanielRuf DanielRuf left a comment

Choose a reason for hiding this comment

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

We should trigger an event to support unobstrusive and asynchronous, reactive implementations.

@kball
Copy link
Contributor

kball commented Aug 12, 2019

This PR is stale and the requested changes were not made. Just going to close it out.

@kball kball closed this Aug 12, 2019
@simongcc
Copy link
Author

@kball Sorry for having not been a while, what should I do with the stale PR?

@kball
Copy link
Contributor

kball commented Aug 13, 2019

@simongcc no worries; the entire project has all been stale for a while, but some of the community is making a push to revive it.

Based on the feedback from the original PR, if you'd like to pick it back up please rework it to trigger an event instead of use a callback, and then reopen/open a new PR.

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

Successfully merging this pull request may close these issues.

3 participants