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(transition): animate all toolbars in iOS page transitions #17224

Merged
merged 30 commits into from Mar 14, 2019

Conversation

@zakton5
Copy link
Contributor

commented Jan 23, 2019

Short description of what this resolves:

Only the first ion-toolbar within an ion-header animates when navigating forward or back.

Changes proposed in this pull request:

  • Add all toolbars within ion-header to the transition instead of just one

Ionic Version: 4.0.0@rc.3

Fixes: #16262

zakton5 and others added some commits Jan 22, 2019

Merge pull request #1 from ionic-team/master
update to latest 1-22-19

@paulstelzer paulstelzer requested a review from manucorporat Jan 23, 2019

paulstelzer and others added some commits Jan 23, 2019

@jayordway

This comment has been minimized.

Copy link

commented Jan 24, 2019

I am excited to see this issue fixed! Hopefully it is merged soon!

@DavidWiesner

This comment has been minimized.

Copy link
Contributor

commented Jan 25, 2019

There is another issue in the animation. This was also an issue before your pull request. What if there is an other element e.g.: an ion-searchbar in one of the toolbars.
A more complex example:

<ion-header>
  <ion-toolbar color="secondary">
    <ion-buttons slot="start">
      <ion-back-button></ion-back-button>
    </ion-buttons>
    <ion-title>SecondPage</ion-title>
    <ion-buttons slot="end">
      <ion-button><ion-icon icon="more"></ion-icon></ion-button>
    </ion-buttons>
  </ion-toolbar>
  <ion-toolbar color="secondary">
    <ion-searchbar></ion-searchbar>
    <ion-buttons slot="end">
      <ion-button><ion-icon icon="funnel"></ion-icon></ion-button>
    </ion-buttons>
  </ion-toolbar>
</ion-header>

Maybe you can fix this issue also in your pull request.
My first attempt for this issue was replacing the ion-buttons,[menuToggle] selector with :scope > :not(ion-title), but I'm not sure I get all the corner cases.

@elmartino

This comment has been minimized.

Copy link

commented Jan 25, 2019

+1

@zakton5

This comment has been minimized.

Copy link
Contributor Author

commented Jan 25, 2019

@DavidWiesner Good catch! I'll play around with the selectors and see what I can find.
I know adding each element to the animation isn't the most efficient, but I'm just extending the logic that was already there. I think the extra overhead to the animation is important to maintain the "feel" of iOS.

@zakton5

This comment has been minimized.

Copy link
Contributor Author

commented Jan 25, 2019

@DavidWiesner Okay, I think I've got it now. It's still a bit inefficient in terms of adding a bunch of stuff to the animation, but it does the job. And now custom elements/components shouldn't be a problem either. It should handle ion-searchbar as well. Here is what it looks like with the most recent updates:

toolbar_transition_2

zakton5 added some commits Jan 25, 2019

@jayordway

This comment has been minimized.

Copy link

commented Jan 27, 2019

Will it handle segment buttons and ion-segment being in there as well?

@zakton5

This comment has been minimized.

Copy link
Contributor Author

commented Jan 28, 2019

@jayordway Yes i just tried it. It handles that as well. Should handle anything as long as it is placed within a toolbar.

zakton5 and others added some commits Jan 30, 2019

Merge pull request #2 from ionic-team/master
Update master 2-1-19

@zakton5 zakton5 changed the title Fix toolbar transition in iOS fix(transition): animate all toolbars in iOS page transitions Feb 8, 2019

zakton5 and others added some commits Feb 16, 2019

Merge pull request #3 from ionic-team/master
Update to latest 2-16-19
@jayordway

This comment has been minimized.

Copy link

commented Feb 22, 2019

When is this getting merged?

liamdebeasi added some commits Mar 14, 2019

@liamdebeasi
Copy link
Member

left a comment

Great job on this! 🎉

@pegler

This comment has been minimized.

Copy link
Contributor

commented Mar 14, 2019

Exciting! Would it be possible to get a dev release when this is merged?

@brandyscarney

This comment has been minimized.

Copy link
Member

commented Mar 14, 2019

Thanks everyone who contributed to this! ❤️ We will definitely get a dev release out once merged.

@liamdebeasi liamdebeasi merged commit 7d01207 into ionic-team:master Mar 14, 2019

1 check passed

build Workflow: build
Details

Ionic Core automation moved this from In progress 🤺 to Done 🎉 Mar 14, 2019

@liamdebeasi

This comment has been minimized.

Copy link
Member

commented Mar 14, 2019

Thank you everyone! 🎉

@liamdebeasi

This comment has been minimized.

Copy link
Member

commented Mar 14, 2019

Hi everyone,

I have pushed a nightly build with these changes (npm i @ionic/core@dev @ionic/angular@dev).

Thanks!

@zakton5 zakton5 deleted the zakton5:fix-toolbar-transition branch Mar 15, 2019

santoshyadav198613 added a commit to santoshyadav198613/ionic that referenced this pull request Mar 16, 2019

fix(transition): animate all toolbars in iOS page transitions (ionic-…
…team#17224)

* fix(transition): add all header toolbars to ios transition

* test(nav): add 2nd toolbar to test transition

* fix(transition): add remaining toolbar children to animation

* fix(transition): fix incorrect variable name

* fix(toolbar): fix bug in safari, clean up code, update test

* fix typo

* change elems to els

* change Elem to El
@jayordway

This comment has been minimized.

Copy link

commented Mar 16, 2019

@liamdebeasi So I have tested with 4.1.2-dev.201903141948.7d01207 and it seems as if this was fixed just for backing out of the view with the "sticky" or static toolbar but not transitioning into it.

@liamdebeasi

This comment has been minimized.

Copy link
Member

commented Mar 18, 2019

Hi @jayordway,

Can you provide the code you used to test this? I can look into it.

Thanks!

@jayordway

This comment has been minimized.

Copy link

commented Mar 19, 2019

@liamdebeasi Due to the nature of the project I cannot make it public, but here is a snippet to show the toolbar section that has the toolbar which appears immediately before the transition/slide even starts:

  <ion-toolbar>
    <ion-buttons slot="start">
      <ion-back-button ></ion-back-button>
    </ion-buttons>
    <ion-title>Ticket details</ion-title>
  </ion-toolbar>
  <ng-container *ngIf="(ticket$ | async) as ticket">
    <ion-toolbar  *ngIf="ticket.enableRequestUpdateTicket || ticket.enableRequestCloseTicket
                               || ticket.enableContestTicket || ticket.enableEscalateTicket" class="details-toolbar">
      <ion-buttons class="ticket-action-buttons">
        <ion-button *ngIf="ticket.enableRequestUpdateTicket " [shape]="'round'" [fill]="'outline'"  (click)="presentModal(actionEnum.UPDATE)">Update</ion-button>
        <ion-button *ngIf="ticket.enableRequestCloseTicket"   [shape]="'round'" [fill]="'outline'"  (click)="presentModal(actionEnum.CLOSE)">Req. close</ion-button>
        <ion-button *ngIf="ticket.enableContestTicket"        [shape]="'round'" [fill]="'outline'"  (click)="presentModal(actionEnum.CONTEST)">Contest</ion-button>
        <ion-button *ngIf="ticket.enableEscalateTicket"       [shape]="'round'" [fill]="'outline'"  (click)="presentModal(actionEnum.ESCALATE)">Escalate</ion-button>
      </ion-buttons>
    </ion-toolbar>
  </ng-container>
</ion-header>

@ionitron-bot ionitron-bot bot added triage and removed needs: reply labels Mar 19, 2019

@liamdebeasi

This comment has been minimized.

Copy link
Member

commented Mar 19, 2019

Hi there,

When ion-button is inside ion-buttons, the ion-button components will not animate/move (they will only fade in/out). This behavior is consistent with what is found in many iOS apps.

If you feel you have found an bug, please create an issue here. Thanks!

@Horst1102

This comment has been minimized.

Copy link

commented Apr 10, 2019

Hi,

i am facing problems with the page transition when coming from a page that has multiple toolbars with segments to a page that also has mutliple toolbars (but one more than the page before).

Head of Page 1 (a tabbed page):

<ion-header>
    <ion-toolbar>
        <ion-buttons slot="start">
            <ion-menu-button></ion-menu-button>
        </ion-buttons>
        <ion-buttons slot="end">
            <ion-button (click)="updateContent()">
                <ion-icon slot="icon-only" name="refresh"></ion-icon>
            </ion-button>
        </ion-buttons>
        <ion-title>{{ 'REPORTER' | translate }}</ion-title>
    </ion-toolbar>
    <ion-toolbar no-border-top>
        <ion-segment [(ngModel)]="reporterFilter">
            <ion-segment-button value="all">
                {{ 'ALL' | translate }}
            </ion-segment-button>
            <ion-segment-button value="unconfirmed">
                {{ 'UNSEEN' | translate }}
            </ion-segment-button>
        </ion-segment>
    </ion-toolbar>
</ion-header>

Head of Page 2 (opened from the tab page):

<ion-header>
    <ion-toolbar>
        <ion-buttons slot="start">
            <ion-back-button text=""></ion-back-button>
        </ion-buttons>
        <ion-buttons slot="end">
            <ion-button
                    (click)='navigate(reporterDetailResponse.street + ", " + reporterDetailResponse.postcode + ", " + reporterDetailResponse.city)'>
                <ion-icon slot="icon-only" name="navigate"></ion-icon>
            </ion-button>
            <ion-button (click)="presentFilter()">
                <ion-icon slot="icon-only" name="options"></ion-icon>
            </ion-button>
        </ion-buttons>
        <ion-title>{{'SINGLEREPORTER' | translate}}</ion-title>
    </ion-toolbar>

    <ion-toolbar>
        <ion-segment [(ngModel)]="segmentAnlagenDetail">
![ezgif-3-9cf88866f761](https://user-images.githubusercontent.com/49471647/55857796-5dd0b580-5b6e-11e9-95a1-fbf9440dfc20.gif)


            <ion-segment-button value="alarms">
                {{ 'ALARMS' | translate }}
            </ion-segment-button>
            <ion-segment-button value="comments">
                {{ 'COMMENTS' | translate }}
            </ion-segment-button>
        </ion-segment>
    </ion-toolbar>

    <ion-toolbar no-padding>
        <div class="filter-item filter-caption">{{'FILTER'| translate}}:</div>
        <div *ngFor="let level of getExcludedLevel()">
            <ion-badge class="filter-item" (click)="removeLevel(level)">{{level.name | translate}}</ion-badge>
        </div>
        <ion-badge class="filter-item" *ngIf="fromDate" (click)="removeDate()">
            <ion-icon name="calendar"></ion-icon>
        </ion-badge>
        <ion-badge class="filter-item" *ngIf="userOnly" (click)="removeUserOnly()">
            <ion-icon name="person"></ion-icon>
        </ion-badge>
    </ion-toolbar>

</ion-header>

So my problem is the following:

  • when changing from page 1 to page 2 the first list item of the content from page 1 is shown in the third toolbar of page 2 while animating (fading in/fasding out).
  • when changing back to page 1 from page 2 the third toolbar ist shown in the first list item ...

I have added a gif for you.
ezgif-3-9cf88866f761

How can i prevent this?

@jayordway

This comment has been minimized.

Copy link

commented Apr 10, 2019

@Horst1102 Didn't Liam state in the previous comment that ion-buttons will not move with the transition?

@Horst1102

This comment has been minimized.

Copy link

commented Apr 10, 2019

@jayordway

Yes, i have read that. But as far as i understand i do not use ion-buttons in the third toolbar.

The element that overlaps is no button: <div class="filter-item filter-caption">{{'FILTER'| translate}}:</div>

@liamdebeasi

This comment has been minimized.

Copy link
Member

commented Apr 10, 2019

Hi there,

If you feel you have found a bug, please open a new Bug Report.

Kiku-git added a commit to Kiku-git/ionic that referenced this pull request May 16, 2019

fix(transition): animate all toolbars in iOS page transitions (ionic-…
…team#17224)

* fix(transition): add all header toolbars to ios transition

* test(nav): add 2nd toolbar to test transition

* fix(transition): add remaining toolbar children to animation

* fix(transition): fix incorrect variable name

* fix(toolbar): fix bug in safari, clean up code, update test

* fix typo

* change elems to els

* change Elem to El
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.