Skip to content

fix(header): collapsible header works in tabs#19658

Merged
liamdebeasi merged 6 commits into
masterfrom
large-header-tabs
Oct 17, 2019
Merged

fix(header): collapsible header works in tabs#19658
liamdebeasi merged 6 commits into
masterfrom
large-header-tabs

Conversation

@liamdebeasi

@liamdebeasi liamdebeasi commented Oct 14, 2019

Copy link
Copy Markdown
Contributor

Pull request checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes / features)
  • Build (npm run build) was run locally and any changes were pushed
  • Lint (npm run lint) has passed locally and any fixes were made for failures

Pull request type

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe):

What is the current behavior?

Issue Number: #19640

What is the new behavior?

  • Header now selects correct tab
  • Also reduced the number of DOM reads/rights by moving the toolbar border animation to the intersection observer

Does this introduce a breaking change?

  • Yes
  • No

Other information

@ionitron-bot ionitron-bot Bot added the package: core @ionic/core package label Oct 14, 2019
Comment thread core/src/components/header/header.tsx Outdated
* border as the top-most toolbar collapses or expands.
*/
const toolbarIntersection = (ev: any) => { handleToolbarIntersection(ev, mainHeaderIndex, scrollHeaderIndex); };
this.intersectionObserver = new IntersectionObserver(toolbarIntersection, { threshold: [0.25, 0.3, 0.4, 0.5, 0.6, 0.7, 0.8, 0.9, 1], rootMargin: `-${mainHeaderHeight}px 0px 0px 0px` });

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The IntersectionObserver API is available as of iOS 12.2, will there be a fallback for earlier versions of iOS?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably not - The polyfill for this is not great. I'll add some feature detection to disable it so users don't get errors. If unsupported, would you be fine with collapse="condense" on the header and collapse="true" on buttons being ignored?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK I see, I have to check how the look and feel is on a device where its disabled but my first thought would be to hide the collapsable - big - header entirely on devices that do not support it and just to display the small header.

@DavidStrausz DavidStrausz Oct 15, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I tried the current behaviour on an iOS 11 device, currently only the large title in the collapsible header is visible and simply scrolls out of view, but because of the missing IntersectionObserver API (which throws an error currently) the small title is never displayed. I think this is acceptable but having it the other way round (i.e. small title in header is always visible, large title never shows) would be nicer as it was the default until iOS 13 anyway.

And its now working inside tabs too!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok cool. I'll do the feature detection in a separate PR so we can get this bug fix out. 👍

@liamdebeasi liamdebeasi merged commit f69f9e4 into master Oct 17, 2019
@liamdebeasi liamdebeasi deleted the large-header-tabs branch October 17, 2019 16:59
liamdebeasi added a commit that referenced this pull request Oct 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

package: core @ionic/core package

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants