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

WIP: slidebox #2288

Closed
wants to merge 1 commit into from
Closed

WIP: slidebox #2288

wants to merge 1 commit into from

Conversation

ajoslin
Copy link
Contributor

@ajoslin ajoslin commented Sep 24, 2014

Test is in test/html/slidebox.html

Benefits of refactor:

  • Only current, previous, and next element are ever in the DOM
  • Only the scope of the current element is ever connected to $watches, events, etc
  • Abstraction of swiping between elements is open for tabs with almost no work, and also sidemenu and views with a bit of work.

TODO

  • Lower the transitionDuration depending on user's drag velocity.
  • Finish documentation
  • Fix CSS height problem
  • Unit test utils/list
  • Cover case with only two slides and previous

@ajoslin ajoslin force-pushed the wip-slidebox branch 6 times, most recently from ba10540 to 0ba5876 Compare September 28, 2014 00:40
@mlynch
Copy link
Contributor

mlynch commented Sep 28, 2014

Nice! One thought as we test that I want to make sure we do right is carrying the velocity with the swipe. slide.js did a good job at this, but if you "throw" quickly, the animation should keep going at that speed. Some slide components "catch" the throw then do an awkward slide after that (see the Ratchet slider for an example of how not to do it: http://goratchet.com/components/#sliders)

@ajoslin
Copy link
Contributor Author

ajoslin commented Sep 28, 2014

Yeah, it's definitely needed. I'll implement it - it's as simple as transitionDuration = slideWidth / velocity

On Sep 28, 2014, at 6:59 AM, Max Lynch [email protected] wrote:

Nice! One thought as we test that I want to make sure we do right is carrying the velocity with the swipe. slide.js did a good job at this, but if you "throw" quickly, the animation should keep going at that speed. Some slide components "catch" the throw then do an awkward slide after that (see the Ratchet slider for an example of how not to do it: http://goratchet.com/components/#sliders)


Reply to this email directly or view it on GitHub.

@ajoslin ajoslin force-pushed the wip-slidebox branch 3 times, most recently from 8765f41 to 2bb8e5b Compare September 28, 2014 22:56
@ajoslin
Copy link
Contributor Author

ajoslin commented Sep 28, 2014

This is about done. It's very modular, and as a result it's almost ready to be brought into tabs.

There's a logic in SlideController that could be moved out into something more reusable. Once that is done, tabs can be refactored for swipeability and these performance improvements.

@ajoslin ajoslin force-pushed the wip-slidebox branch 3 times, most recently from 7ef84bd to dcac7f9 Compare September 28, 2014 23:49
@chrismoutray
Copy link

Was wondering about the todo item "Fix CSS height problem".
I guess this is to do with the issue where all slides have the height of the tallest slide. eg one slide has scrollable content... a long list, and other slides are forced to have that height too.

Will it be possible to setup the slides height so that it is always as tall as the screen (minus the height of header and other content outside of the slide)?
I have a problem at the moment where I want to allow the user to swipe between slides even if the content of the slide only takes say a 1/5 of the screens height. I want them to swipe to the next slide even if they swipe at the bottom of the screen (outside of the slides content).
With current implementation I set the height to 100% but this broke the slides scroll feature ie for when a slide had more items than the screens height.

Hope this makes sense, and again looking forward to this update :)

@ajoslin
Copy link
Contributor Author

ajoslin commented Oct 7, 2014

Hi Chris,

That problem may be fixed now. I have some new changes to push, I'll test out your use case.

On Oct 7, 2014, at 13:21, Chris Moutray [email protected] wrote:

Was wondering about the todo item "Fix CSS height problem".
I guess this is to do with the issue where all slides have the height of the tallest slide. eg one slide has scrollable content... a long list, and other slides are forced to have that height too.

Will it be possible to setup the slides height so that it is always as tall as the screen (minus the height of header and other content outside of the slide)?
I have a problem at the moment where I want to allow the user to swipe between slides even if the content of the slide only takes say a 1/5 of the screens height. I want them to swipe to the next slide even if they swipe at the bottom of the screen (outside of the slides content).
With current implementation I set the height to 100% but this broke the slides scroll feature ie for when a slide had more items than the screens height.

Hope this makes sense, and again looking forward to this update :)


Reply to this email directly or view it on GitHub.

@four2theizz0
Copy link

Have to add I am eagerly awaiting this as well. I am in the same boat as Chris :)

@ajoslin
Copy link
Contributor Author

ajoslin commented Oct 8, 2014

The slidebox now alwayss take up the size of its parent scroll container. This is ready for review.

Closes #2336. Closes #2317. Closes #2290. Closes #2228. Closes #2067.
Closes #1890. Closes #1865. Closes #1850. Closes #1755. Closes #1688.
Closes #1578. Closes #1501. Closes #1353. Closes #1342. Closes #782.
Closes #416. Closes #2288.

BREAKING CHANGE: The slideBox's API has undergone many changes.

- **`<ion-slide-box>`** attributes have changed (see
  [documentation](http://ionicframework.com/docs/api/directive/ionSlideBox)):

  * `active-slide` has changed to `selected`. Change your code from
  this:

    ```html
    <ion-slide-box active-slide="activeSlideIndex"></ion-slide-box>
    ```

    To this:

    ```html
    <ion-slide-box selected="activeSlideIndex"></ion-slide-box>
    ```

  * `does-continue` has changed to `loop`.  Change your code from this:

    ```html
    <ion-slide-box does-continue="shouldLoop"></ion-slide-box>
    ```

    To this:

    ```html
    <ion-slide-box loop="shouldLoop"></ion-slide-box>
    ```

  * `auto-play` and `slide-interval` have been merged into `auto-play`.
  Change your code from this:

    ```html
    <!-- autoPlay is on -->
    <ion-slide-box auto-play="true" slide-interval="1000">
    </ion-slide-box>
    <!-- autoPlay is off -->
    <ion-slide-box auto-play="false" slide-interval="1000">
    </ion-slide-box>
    ```

    To this:

    ```html
    <!-- autoPlay is on -->
    <ion-slide-box auto-play="1000"></ion-slide-box>
    <!-- autoPlay is off -->
    <ion-slide-box auto-play="false"></ion-slide-box>
    ```

  * `show-pager` and `pager-click` have been removed. Use
  a child `<ion-slide-pager>` element. See the [`ion-slide-pager`
  documentation](http://ionicframework.com/docs/api/directive/ionSlidePager).
  Change your code from this:

  ```html
  <!-- pager using default click action -->
  <ion-slide-box show-pager="true">
  </ion-slide-box>
  <!-- pager with custom click action -->
  <ion-slide-box show-pager="true" pager-click="doSomething(index)">
  </ion-slide-box>
  ```

  To this:

  ```html
  <ion-slide-box>
    <!-- pager using default click action -->
    <ion-slide-pager></ion-slide-pager>
  </ion-slide-box>
  <ion-slide-box>
    <!-- pager with custom click action -->
    <ion-slide-pager ng-click="doSomething(index)"></ion-slide-pager>
  </ion-slide-box>
  ```

- **`$ionicSlideBoxDelegate`** methods have changed (see
  [documentation](http://ionicframework.com/docs/api/service/$ionicSlideBoxDelegate)):

  - `update()` has been removed. slideBox updates on its own now.

  - `stop()` has been removed. See `autoPlay()` below.

  - `start()` hass been removed. See `autoPlay()` below.

  - `slide(newIndex[, speed])` has been renamed to `select(newIndex[,
    speed]);

  - `currentIndex()` has been renamed to `selected()`.

  - `slidesCount()` has been renamed to `count()`.

  - New method `$ionicSlideBoxDelegate.autoPlay()`. Change your code
    from this:

    ```js
    // stop auto sliding
    $ionicSlideBoxDelegate.stop();
    // later... start auto sliding
    $ionicSlideBoxDelegate.start();
    ```

    To this:

    ```js
    var autoPlaySpeed = 3000; //wait 3000 seconds between changing slide
    // stop auto sliding
    $ionicSlideBoxDelegate.autoPlay(false);
    // later... start auto sliding
    $ionicSlideBoxDelegate.autoPlay(autoPlaySpeed);
    ```

  - `previous()` now returns the index of the previous slide and does
    not select. Change your code from this:

    ```js
    // select previous slide
    $ionicSlideBoxDelegate.previous();
    ```

    To this:

    ```js
    // select previous slide
    $ionicSlideBoxDelegate.select( $ionicSlideBoxDelegate.previous() );
    ```
  - `next()` now returns the index of the next slide and does
    not select. Change your code from this:

    ```js
    // select next slide
    $ionicSlideBoxDelegate.next();
    ```

    To this:

    ```js
    // select next slide
    $ionicSlideBoxDelegate.select( $ionicSlideBoxDelegate.next() );
    ```
@redeemedfadi
Copy link

@ajoslin This is great!

However, I can't seem to set a start slide. In test/html/slideBox.html adding the following:

$scope.$root.selectedIndex = 5;

doesn't make a difference. This is because this $watch expression happens after this variable is already set. Executing SlideBoxCtrl.select at that point doesn't seem to work because slides are added afterwards.

The other issue I noticed was loading performance. The slide box takes ~500ms to load on a top of the line Macbook Pro. It takes several seconds on a Nexus 5. I'm not exactly sure what's going on internally with Angular, but Request Animation Frame is being fired with each item in the list and then a frame is fired for each item afterwards. See attached timeline:

screen shot 2014-10-08 at 9 08 02 pm

Here Is the timeline export from Chrome DevTools.

Thanks for all your hard work!

@ajoslin
Copy link
Contributor Author

ajoslin commented Oct 9, 2014

@redeemedfadi: the load-time is because the slideBox test has a 1000-item ng-repeat.

Also, be sure to run $scope.$apply after changing selectedIndex! (shortcut: $scope.$apply('$root.selectedIndex = 5');

The $watch you see in the code is on the isolate scope's selectedIndex variable, which is set when the parent scope's attribute changes.

@ajoslin
Copy link
Contributor Author

ajoslin commented Oct 9, 2014

..And the requestAnimationFrames look to be from angular-animate running an ng-repeat enter animation for every slide element.

@redeemedfadi
Copy link

@ajoslin Thanks for the response. I still can't get the slide box to open on a certain slide. Changing a slide by modifying selectedIndex works fine, but I want to open/initialize on a certain slide.

@ajoslin
Copy link
Contributor Author

ajoslin commented Oct 9, 2014

@redeemedfadi I see. OK, this can be done.

ajoslin added a commit that referenced this pull request Oct 9, 2014
@ajoslin
Copy link
Contributor Author

ajoslin commented Oct 9, 2014

@redeemedfadi OK, added. Try out the nightly builds.

The slideBox now allows you to set the selected binding to values that are currently outside of the slideBox's current boundaries (eg you can set selected to 3 when there are 0 slides available).

Then, whenever a new slide is added, it will check if that slide's new index matches what should be the selected index. If so, that slide will be selected.

@redeemedfadi
Copy link

Thanks @ajoslin for the quick update! That works great.

I also added an ng-if to the slide to greatly reduce the load time with a large number of slides:

<ion-slide ng-repeat="i in items" ng-if="$index <= $root.selectedIndex + 2 && $index >= $root.selectedIndex - 2">

This should probably be part of the ionSlide directive, but I'm not familiar enough with angular to know exactly where in the directive it should go.

Of course, it doesn't work quite right since the index - 2 needs to wrap around.

@ajoslin
Copy link
Contributor Author

ajoslin commented Oct 9, 2014

@fadi unfortunately it's not that simple, since on-demand compiling of a
large slide will cause lag.

In the future, we'll have to do something similar to collection-repeat. I
wanted to allow ng-if and ng-repeat normally, because people already use
them all the time in the slide boxes.

On Thu, Oct 9, 2014 at 9:11 AM, Fadi Eliya [email protected] wrote:

Thanks @ajoslin https://github.com/ajoslin for the quick update! That
works great.

I also added an ng-if to the slide to greatly reduce the load time with a
large number of slides:

This should probably be part of the ionSlide directive, but I'm not
familiar enough with angular to know exactly where in the directive it
should go.

Of course, it doesn't work quite right since the index - 2 needs to wrap
around.


Reply to this email directly or view it on GitHub
#2288 (comment).

@redeemedfadi
Copy link

@ajoslin The on-slide-changed attribute doesn't execute when swiping. It only works when using the select function. Also, even when using the select function the $index or $newSlide variable is undefined.

I added this to slideBox.html example to test:

<ion-slide-box on-slide-changed="onSlideChanged($slideIndex)">
          $scope.onSlideChanged = function($index){
            console.log($index);
          }

Thanks!

@redeemedfadi
Copy link

@ajoslin I noticed you fixed the on-slide-changed attribute. Thanks!

This may be premature, but just wanted to let you know if you didn't notice that all the ion-slides are now being rendered whether or not their state is 'detached'.

@redeemedfadi
Copy link

Another bug I found: neither $ionicSlideBoxDelegate.next() or SlideBoxCtrl.next() work. Same with previous.

@marcsyp
Copy link

marcsyp commented Oct 9, 2014

Is everybody reading the migration notes? If you open the nightlies docs
you can see them. There are a lot of tractor properties and methods, those
included.
On Oct 9, 2014 3:22 PM, "Fadi Eliya" [email protected] wrote:

Another bug I found: neither $ionicSlideBoxDelegate.next() or
SlideBoxCtrl.next() work. Same with previous.


Reply to this email directly or view it on GitHub
#2288 (comment).

@redeemedfadi
Copy link

Ahh sorry I missed that one. I did read the commit notes but forgot about that method.

ajoslin added a commit that referenced this pull request Oct 10, 2014
Closes #2336. Closes #2317. Closes #2290. Closes #2228. Closes #2067.
Closes #1890. Closes #1865. Closes #1850. Closes #1755. Closes #1688.
Closes #1578. Closes #1501. Closes #1353. Closes #1342. Closes #782.
Closes #416. Closes #2288.

BREAKING CHANGE: The slideBox's API has undergone many changes.

- **`<ion-slide-box>`** attributes have changed (see
  [documentation](http://ionicframework.com/docs/api/directive/ionSlideBox)):

  * `active-slide` has changed to `selected`. Change your code from
  this:

    ```html
    <ion-slide-box active-slide="activeSlideIndex"></ion-slide-box>
    ```

    To this:

    ```html
    <ion-slide-box selected="activeSlideIndex"></ion-slide-box>
    ```

  * `does-continue` has changed to `loop`.  Change your code from this:

    ```html
    <ion-slide-box does-continue="shouldLoop"></ion-slide-box>
    ```

    To this:

    ```html
    <ion-slide-box loop="shouldLoop"></ion-slide-box>
    ```

  * `auto-play` and `slide-interval` have been merged into `auto-play`.
  Change your code from this:

    ```html
    <!-- autoPlay is on -->
    <ion-slide-box auto-play="true" slide-interval="1000">
    </ion-slide-box>
    <!-- autoPlay is off -->
    <ion-slide-box auto-play="false" slide-interval="1000">
    </ion-slide-box>
    ```

    To this:

    ```html
    <!-- autoPlay is on -->
    <ion-slide-box auto-play="1000"></ion-slide-box>
    <!-- autoPlay is off -->
    <ion-slide-box auto-play="false"></ion-slide-box>
    ```

  * `show-pager` and `pager-click` have been removed. Use
  a child `<ion-slide-pager>` element. See the [`ion-slide-pager`
  documentation](http://ionicframework.com/docs/api/directive/ionSlidePager).
  Change your code from this:

  ```html
  <!-- pager using default click action -->
  <ion-slide-box show-pager="true">
  </ion-slide-box>
  <!-- pager with custom click action -->
  <ion-slide-box show-pager="true" pager-click="doSomething(index)">
  </ion-slide-box>
  ```

  To this:

  ```html
  <ion-slide-box>
    <!-- pager using default click action -->
    <ion-slide-pager></ion-slide-pager>
  </ion-slide-box>
  <ion-slide-box>
    <!-- pager with custom click action -->
    <ion-slide-pager ng-click="doSomething(index)"></ion-slide-pager>
  </ion-slide-box>
  ```

- **`$ionicSlideBoxDelegate`** methods have changed (see
  [documentation](http://ionicframework.com/docs/api/service/$ionicSlideBoxDelegate)):

  - `update()` has been removed. slideBox updates on its own now.

  - `stop()` has been removed. See `autoPlay()` below.

  - `start()` hass been removed. See `autoPlay()` below.

  - `slide(newIndex[, speed])` has been renamed to `select(newIndex[,
    speed]);

  - `currentIndex()` has been renamed to `selected()`.

  - `slidesCount()` has been renamed to `count()`.

  - New method `$ionicSlideBoxDelegate.autoPlay()`. Change your code
    from this:

    ```js
    // stop auto sliding
    $ionicSlideBoxDelegate.stop();
    // later... start auto sliding
    $ionicSlideBoxDelegate.start();
    ```

    To this:

    ```js
    var autoPlaySpeed = 3000; //wait 3000 seconds between changing slide
    // stop auto sliding
    $ionicSlideBoxDelegate.autoPlay(false);
    // later... start auto sliding
    $ionicSlideBoxDelegate.autoPlay(autoPlaySpeed);
    ```

  - `previous()` now returns the index of the previous slide and does
    not select. Change your code from this:

    ```js
    // select previous slide
    $ionicSlideBoxDelegate.previous();
    ```

    To this:

    ```js
    // select previous slide
    $ionicSlideBoxDelegate.select( $ionicSlideBoxDelegate.previous() );
    ```
  - `next()` now returns the index of the next slide and does
    not select. Change your code from this:

    ```js
    // select next slide
    $ionicSlideBoxDelegate.next();
    ```

    To this:

    ```js
    // select next slide
    $ionicSlideBoxDelegate.select( $ionicSlideBoxDelegate.next() );
    ```
@bjammin
Copy link

bjammin commented May 25, 2015

So this never made it into 1.0 ?

@subeebot
Copy link

@ajoslin Any updates to this, and in particular re: lazy loading of slides?

@bjammin
Copy link

bjammin commented May 28, 2015

Yes that's especially what I'm interested in @subeebot

I have a large slidebox and sometimes I transition to the view and slide to (for example) slide 100. Unfortunately it loads slides 1-99 as well resulting in a large delay. I'd love to see "Only current, previous, and next element are ever in the DOM" but it seems this pull request was never accepted :(

@marcsyp
Copy link

marcsyp commented May 28, 2015

Yeah, I'm still dealing with huge load times that make my app appear
amateurish. :(

On Wed, May 27, 2015 at 11:58 PM, bjammin [email protected] wrote:

Yes that's especially what I'm interested in @subeebot
https://github.com/subeebot

I have a large slidebox and sometimes I transition to the view and slide
to (for example) slide 100. Unfortunately it loads slides 1-99 as well
resulting in a large delay. I'd love to see "Only current, previous, and
next element are ever in the DOM" but it seems this pull request was never
accepted :(


Reply to this email directly or view it on GitHub
#2288 (comment).

Marc Syp, Creative Technologist
San Francisco, CA

WARNING: The information contained in this message is legally
privileged and proprietary information intended only for the use of
the individual or entity named above. If the reader of this message is
not the intended recipient, you are hereby notified that any
dissemination, distribution, or copy of this message is strictly
prohibited. If you have received this message in error, please
immediately delete the original message and notify the sender.
Communication of the information contained in this message to any
unauthorized persons may be a violation of state or federal laws.

@bjammin
Copy link

bjammin commented May 29, 2015

I did find this post:
http://forum.ionicframework.com/t/what-should-i-do-if-i-want-to-show-thousands-of-pages-in-a-slide-box/14092

It discusses some methods (code included) to implement your own lazy loading. I'll be going down this path I guess.

@marcsyp
Copy link

marcsyp commented May 29, 2015

Thank you for that! Will give it a try when I have a chance and report
back here.

Marc

On Thu, May 28, 2015 at 5:05 PM, bjammin [email protected] wrote:

I did find this post:

http://forum.ionicframework.com/t/what-should-i-do-if-i-want-to-show-thousands-of-pages-in-a-slide-box/14092

It discusses some methods (code included) to implement your own lazy
loading. I'll be going down this path I guess.


Reply to this email directly or view it on GitHub
#2288 (comment).

Marc Syp, Creative Technologist
San Francisco, CA

WARNING: The information contained in this message is legally
privileged and proprietary information intended only for the use of
the individual or entity named above. If the reader of this message is
not the intended recipient, you are hereby notified that any
dissemination, distribution, or copy of this message is strictly
prohibited. If you have received this message in error, please
immediately delete the original message and notify the sender.
Communication of the information contained in this message to any
unauthorized persons may be a violation of state or federal laws.

@bjammin
Copy link

bjammin commented Jun 5, 2015

I was trying to use slidebox to re-create this image carousel as pure ionic:
https://blueimp.github.io/Gallery/

Unfortunately I haven't been able to get near the performance of that component, even with a small number of slides. So I have been forced to adapt blueimp instead. It's a bit messy with no direct binding but I have managed to work around it and the performance is streets ahead of slidebox.

I guess slidebox is trying to cope with a lot of different use cases so it's not as well optimised as an image carousel.

@vandervidi
Copy link

Is there any working solution for lazy loading of slides?

@5amfung
Copy link

5amfung commented Dec 8, 2015

Any update on this?

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.

10 participants