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

Fix #841 Don't wrap ion-content content in a .scroll element #897

Closed
wants to merge 2 commits into from
Closed

Fix #841 Don't wrap ion-content content in a .scroll element #897

wants to merge 2 commits into from

Conversation

jayproulx
Copy link

Fix for #841

Downside is that the scroll property is immutable, unless you've got a clean way to $compile on a scope change. Is this likely to be a real-world scenario?

@ajoslin
Copy link
Contributor

ajoslin commented Mar 25, 2014

Looks good! And well tested :-)

I don't think we can support the re-compilation, and that's OK.

Could you add something to the docs for the scroll attribute to note this? Something like 'Note: attribute is not watched - no-scroll effect happens upon element compilation'

https://github.com/jayproulx/ionic/blob/fix-841/js/ext/angular/src/directive/ionicContent.js#L51

@jayproulx
Copy link
Author

Done

@ajoslin
Copy link
Contributor

ajoslin commented Mar 25, 2014

Thanks @jayproulx! We're cutting a release now, I'll wait to merge it until after that, since it is a 'risky' change.

@ajoslin ajoslin self-assigned this Mar 25, 2014
@adamdbradley adamdbradley added this to the 1.0.0-beta.2 milestone Mar 26, 2014
@ajoslin
Copy link
Contributor

ajoslin commented Mar 27, 2014

final-reviewing this for merge now now.

@ajoslin ajoslin closed this in 73da93d Mar 27, 2014
@ajoslin
Copy link
Contributor

ajoslin commented Mar 27, 2014

I got sidetracked and forgot to finish merging it. Done!

Thanks @jayproulx! keep it up. Awesome PR, with questions asked, tests written, and followup accomplished.

@jayproulx
Copy link
Author

Hey, awesome framework, I'm glad to contribute. I expect we'll be using it quite a bit in the future!

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