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

ion-content scroll="false" still wraps content in scroll elements #841

Closed
jayproulx opened this issue Mar 19, 2014 · 18 comments
Closed

ion-content scroll="false" still wraps content in scroll elements #841

jayproulx opened this issue Mar 19, 2014 · 18 comments
Milestone

Comments

@jayproulx
Copy link

Documentation:
http://ionicframework.com/docs/angularjs/views/content/

Setting scroll="false" on ion-content should prevent scrolling behaviours (and allow "full screen content")

Forked from one of the ion examples:
http://codepen.io/anon/pen/mEyBn

Expected:

<ion-content><div class="mycontent">My Content</div></ion-content>

Actual:

<ion-content class="scroll-container"><div class="scroll"><div class="mycontent">My Content</div></div></ion-content>

Is this a known issue?

@mlynch
Copy link
Contributor

mlynch commented Mar 20, 2014

Hey @jayproulx, the scroll="false" case is not actually documented and this is not the intention of it. We need to fix the docs.

If you wish to have a full screen content area, just don't use <ion-content> and keep it in a <ion-pane>. Thanks!

@mlynch mlynch closed this as completed Mar 20, 2014
@jayproulx
Copy link
Author

Can we look at re-openning this bug? ion-pane doesn't seem to have the functionality I'm looking for (See #843). I can build out the solution, just let me know what approach you'd like me to take and I'll update the pull request.

@ajoslin
Copy link
Contributor

ajoslin commented Mar 24, 2014

@jayproulx So you're looking for an ion-content without scroll, but still with the has-header/etc classes?

@jayproulx
Copy link
Author

Yup! Need a full screen view that respects the headers and footers (ie. a nicely spaced flexbox view stretching from inside the header to inside the footer). Maybe you've got another solution already built and I've been searching for the wrong thing.

@ajoslin
Copy link
Contributor

ajoslin commented Mar 24, 2014

For now, if you use ion-pane with 'has-header' and 'has-footer' classes it will work. It's not documented, however.

I think reopening this issue would be worth doing, so that people could use ion-content without scroll and get the niceness of header/footer fitting automatically. @mlynch thoughts?

@jayproulx
Copy link
Author

So, I tried out ion-pane with has-header, and I'm not getting that behaviour... thoughts?

http://codepen.io/anon/pen/xusel

@ajoslin
Copy link
Contributor

ajoslin commented Mar 24, 2014

two problems there:

  • Your problem: You used has-header attribute instead of class
  • Our problem: the .pane class overrides the has-header class!

For now (until the next release), you will have to create classes of your own that have top: 44px; and bottom: 44px.

Will fix this by beta (in the coming days), in an easy way.

@ajoslin ajoslin reopened this Mar 24, 2014
@ajoslin ajoslin added this to the 1.0.0-beta.1 milestone Mar 24, 2014
@jayproulx
Copy link
Author

HAH, ok, my bad :) I kept the has-header attribute like i had on ion-content. I could send a pull request for ion-pane that converts the has-header attr to a class, and add a .pane.has-header style that adds the 44px top. that should be more specific and override the .pane definition as well as avoiding any backwards compatibility (can also add the appropriate styles for ios7)

that way, it should mimic the ion-content API to keep the documentation equivalent.

@ajoslin
Copy link
Contributor

ajoslin commented Mar 24, 2014

In the coming release, we actually have removed the has-* attributes altogether from content and use classNames instead.

And then additionally, ion-content automatically adds & removes these classes based on what is surrounding it.

So for the next release, ideally:

  1. The CSS classes will work!
  2. ion-pane will automatically add/remove the classes itself (borrowing from ion-content).

@ajoslin
Copy link
Contributor

ajoslin commented Mar 24, 2014

This would be easy to do, in a couple steps, if you want to open a PR (don't feel obligated to though!)

  1. Factor this out into a function: https://github.com/driftyco/ionic/blob/master/js/ext/angular/src/directive/ionicContent.js#L82-L92
  2. Add it to ion-pane directive: https://github.com/driftyco/ionic/blob/master/js/ext/angular/src/directive/ionicContent.js#L21
  3. Fix the css class specificity or order, which @adamdbradley knows about if you have questions

@jayproulx
Copy link
Author

Great, I'll take this on

@ajoslin
Copy link
Contributor

ajoslin commented Mar 25, 2014

I actually realized we don't want this in ion-pane automatically ... ion-pane is very often used in places where it should not be offset for headers and the like (eg <ion-pane ion-side-menu-content>)

@ajoslin
Copy link
Contributor

ajoslin commented Mar 25, 2014

So instead ... we can go back your original idea, and make scroll="false" not wrap ion-content in scroll elements.

A recent refactor actually made this easily possible.

@jayproulx
Copy link
Author

From an implementation perspective, are there any use cases that should be considered in unit testing to ensure that unwrapping won't adversely affect other implementations? I can't think of any from my limited exposure, but I can write the tests for them if you can think of any.

@ajoslin
Copy link
Contributor

ajoslin commented Mar 25, 2014

Implementation would be:

  1. Put an if (attr.scroll != 'false') around here: https://github.com/driftyco/ionic/blob/master/js/ext/angular/src/directive/ionicContent.js#L69-L74
  2. Make this (innerElement || $element).addClass: https://github.com/driftyco/ionic/blob/master/js/ext/angular/src/directive/ionicContent.js#L109
  3. Add a unit test for padding="true" & scroll="false"
  4. Add a unit test for scroll="false" & the layout
  5. Add unit tests for anything else you can see

@ajoslin
Copy link
Contributor

ajoslin commented Mar 25, 2014

Thanks @jayproulx :)

@jayproulx
Copy link
Author

Just ignore the pull request from my master branch, it had a merge in it, which I've removed by creating a feature branch with only the relevant changes.

@ionitron-bot
Copy link

ionitron-bot bot commented Sep 6, 2018

Thanks for the issue! This issue is being locked to prevent comments that are not relevant to the original issue. If this is still an issue with the latest version of Ionic, please create a new issue and ensure the template is fully filled out.

@ionitron-bot ionitron-bot bot locked and limited conversation to collaborators Sep 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants