ionic-team / ionic-framework

A powerful cross-platform UI toolkit for building native-quality iOS, Android, and Progressive Web Apps with HTML, CSS, and JavaScript.
https://ionicframework.com
MIT License
50.94k stars 13.52k forks source link

Nesting multiple <ion-content> with <ion-slides> #10258

Closed hotforfeature closed 7 years ago

hotforfeature commented 7 years ago

Ionic version: (check one with "x") [ ] 1.x [x] 2.x

I'm submitting a ... (check one with "x") [x] bug report [ ] feature request [ ] support request => Please do not submit support requests here, use one of these channels: https://forum.ionicframework.com/ or http://ionicworldwide.herokuapp.com/

Current behavior: I am using <ion-slides> to display a wizard-like set of components on a single page. The app goes to the slider page and the user will go through different slides on the page. The slides contain forms and inputs, and scroll assist was not working.

I don't believe this usage of <ion-slides> was intended, but I've seen a few other developers use it this way. The advantage is that for long wizards/workflows, there is an option to dismiss the entire page, rather than going back through a stack of several pages. Additionally, it cuts down on the issue of having multiple pages in your navigation stack that you must pass information to. It's possible with NavParams, but it's much simpler to use components in each slide and bind data directly to them. There's no documentation for how to "correctly" do this sort of UX, but I've got a working theory.

First, the slides must be ion-fixed, and each individual slide should have its own <ion-content>. This causes scroll assist to work correctly on individual slides when focusing an input. However, nesting <ion-content>s was never an intended feature, and each child content overrides the ViewController's set Content. This becomes a problem when using iOS transitions.

Bug: content_bug

Expected behavior: content_fix

Steps to reproduce: Create and push a page with multiple <ion-content> using iOS transitions.

Related code:

Here's a sample page setup to push:

<ion-header>
  <ion-navbar color="primary">
    <ion-title>Slider Page</ion-title>
  </ion-navbar>
</ion-header>

<ion-content>
  <!-- Slides must be fixed, otherwise scroll assist adds bottom padding 
  and the slides move off the screen -->
  <ion-slides ion-fixed>

    <!-- With this UX, each "slide" is the main content of the page -->
    <ion-slide>
      <ion-content>
        <form padding>
          <ion-item>
            <ion-label>This Input uses Scroll Assist Correctly</ion-label>
            <ion-input></ion-input>
          </ion-item>
        </form>
      </ion-content>
    </ion-slide>

    <ion-slide>
      <ion-content>
         <!-- More content -->
      </ion-content>
    </ion-slide>

  </ion-slides>
</ion-content>

Here is the utility function I'm using right now to patch ViewController. If other developers are running into this issue, feel free to use this as a workaround. With this patch, I get the expected output GIF above.

/**
 * Patches the ViewController's functions to set its Content. When using a view with
 * multiple <ion-content>, each child content will override the view controller's main content,
 * which causes visual glitches when using iOS page transitions.
 *
 * This patch ensures that only the first <ion-content> registers itself as the view controller's
 * main content.
 *
 * This patch should be done in the constructor of the view that has multiple <ion-content>
 * elements.
 *
 * @param {ViewController} viewCtrl the ViewController to patch.
 */
export function patchViewControllerContent(viewCtrl: ViewController) {
  const _setIONContent = viewCtrl._setIONContent;
  viewCtrl._setIONContent = (...args) => {
    if (!viewCtrl.getIONContent()) {
      _setIONContent.apply(viewCtrl, args);
    }
  };

  const _setIONContentRef = viewCtrl._setIONContentRef;
  viewCtrl._setIONContentRef = (...args) => {
    if (!viewCtrl.getIONContentRef()) {
      _setIONContentRef.apply(viewCtrl, args);
    }
  };
}

// And the Sample Page implementation

export class SamplePage {
  constructor(private viewCtrl: ViewController) {
    patchViewControllerContent(this.viewCtrl);
  }
}

I believe the best solution would be modifying Content's constructor.

export class Content extends Ion implements OnDestroy, OnInit {
  // ...
  constructor(
    config: Config,
    private _plt: Platform,
    private _dom: DomController,
    elementRef: ElementRef,
    renderer: Renderer,
    public _app: App,
    public _keyboard: Keyboard,
    public _zone: NgZone,
    @Optional() viewCtrl: ViewController,
    @Optional() public _tabs: Tabs
  ) {
    super(config, elementRef, renderer, 'content');

    this.statusbarPadding = config.getBoolean('statusbarPadding', false);
    this._imgReqBfr = config.getNumber('imgRequestBuffer', 1400);
    this._imgRndBfr = config.getNumber('imgRenderBuffer', 400);
    this._imgVelMax = config.getNumber('imgVelocityMax', 3);
    this._scroll = new ScrollView(_plt, _dom);

    if (viewCtrl) {
      // content has a view controller
      // ** OLD **
      /*viewCtrl._setIONContent(this);
      viewCtrl._setIONContentRef(elementRef);*/

      // ** FIX **
      if (!viewCtrl.getIONContent()) {
        viewCtrl._setIONContent(this);
        viewCtrl._setIONContentRef(elementRef);
      } else {
        // ViewController already has main Content, this Content instance is a child of a parent content
      }

      this._viewCtrlReadSub = viewCtrl.readReady.subscribe(() => {
        this._viewCtrlReadSub.unsubscribe();
        this._readDimensions();
      });

      this._viewCtrlWriteSub = viewCtrl.writeReady.subscribe(() => {
        this._viewCtrlWriteSub.unsubscribe();
        this._writeDimensions();
      });

    } else {
      // content does not have a view controller
      _dom.read(this._readDimensions.bind(this));
      _dom.write(this._writeDimensions.bind(this));
    }
  }
}

Questions / Discussion

There are a couple other ways to go about it, and I'd like to open this issue as a discussion. This could be considered a new feature in addition to a bug fix.

Are there other developers out there that see this as a valid UX use case? Is it useful enough to add documentation to <ion-slides> to highlight this possible feature? Is there a better way to create a wizard/workflow solution?

Ionic info: (run ionic info from a terminal/cmd prompt and paste output below):

Cordova CLI: 6.5.0 
Ionic Framework Version: 2.0.0
Ionic CLI Version: 2.2.1
Ionic App Lib Version: 2.2.0
Ionic App Scripts Version: 1.0.0
ios-deploy version: 1.9.0 
ios-sim version: 5.0.8 
OS: macOS Sierra
Node Version: v6.7.0
Xcode version: Xcode 8.2.1 Build version 8C1002
jgw96 commented 7 years ago

Hello, thanks for opening an issue with us! You shouldnt actually be using multiple ion-contents in your app. If you would like input scrolling to work correctly in slides you should use this component instead. Also, be aware, as you stated this usage of ion-slides is not standard and therefore i cannot guarantee that after using ion-scroll that everything will work great. As for the last few questions you asked i would recommend asking around on our slack channel or our forum. Thanks for using Ionic!

hotforfeature commented 7 years ago

Thanks for the feedback. For reference, ion-scroll will not work correctly with input scrolling, either inside or outside ion-slides. Inputs pull an ion-content reference to manage their assistive scrolling, so you'll run into the same problem: focusing an input with assist enabled will scroll to the top of the page instead of to the input itself.

I still think this can be a valid usage of ion-slides, as evidenced by #5571. While I can see that the original purpose was for an image carousel, there's no reason to not discuss using form input elements within a slider.

ionitron-bot[bot] commented 6 years ago

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.