Skip to content

Conversation

liamdebeasi
Copy link
Contributor

@liamdebeasi liamdebeasi commented Oct 3, 2023

Issue number: resolves #27060


What is the current behavior?

The main header is controlled by the header with collapse="condense" set:

private async setupCondenseHeader(contentEl: HTMLElement | null, pageEl: Element | null) {

The collapse header will hide the main header and then show it once the user has scrolled enough. However, if the main header is rendered before the collapse header is rendered, then the main header will be visible for a brief moment before being hidden by the collapse header. This gives the perception of flicker that is reported on the linked issue.

What is the new behavior?

  • The main header will be hidden on load if it loads before the collapse header

The selector was written in a way such that once the collapse header loads, this CSS no longer applies (since the collapse header will add .header-collapse-main to the main header)

main branch
RPReplay_Final1696366309.MP4
RPReplay_Final1696366283.MP4

Does this introduce a breaking change?

  • Yes
  • No

Other information

Note: :has browser compat is still fairly new. However, it is available on both Chromium and WebKit browsers (and has been for at least a year): https://caniuse.com/?search=%3Ahas

Given that this bug is a fairly minor UI glitch (as opposed to something that would cause an app to crash or otherwise malfunction), I think this is an acceptable tradeoff. As time goes on this will become less of a concern as more users update their devices.

Dev build: 7.4.3-dev.11696365694.156f41d3

@github-actions github-actions bot added the package: core @ionic/core package label Oct 3, 2023
@liamdebeasi liamdebeasi marked this pull request as ready for review October 3, 2023 20:55
@liamdebeasi liamdebeasi requested review from a team and thetaPC and removed request for a team October 3, 2023 21:02
@thetaPC
Copy link
Contributor

thetaPC commented Oct 4, 2023

I'm still seeing it with the dev build in one of the community's repo. The flickering is happening on the name.

I couldn't replicate the issue on a fresh starter though. I didn't see anything out of place in the community's repo.

Screen.Recording.2023-10-04.at.11.45.53.AM.mov
@liamdebeasi
Copy link
Contributor Author

That doesn't look like the same issue. This PR addresses an issue when the main header loads before the collapsing header, but in your demo the collapsing header is already loaded. I didn't look too much into why it was happening, but I did verify that the behavior is not being caused by my dev build.

@thetaPC
Copy link
Contributor

thetaPC commented Oct 4, 2023

That doesn't look like the same issue. This PR addresses an issue when the main header loads before the collapsing header, but in your demo the collapsing header is already loaded. I didn't look too much into why it was happening, but I did verify that the behavior is not being caused by my dev build.

Do we have a ticket tracking this other issue?

@liamdebeasi
Copy link
Contributor Author

liamdebeasi commented Oct 4, 2023

No, but I'm also not confident this is an Ionic bug. I wasn't able to reproduce on my end in an Ionic starter app. (Though I can reproduce the behavior in the user's app)

Copy link
Contributor

@thetaPC thetaPC left a comment

Choose a reason for hiding this comment

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

LGTM

@liamdebeasi liamdebeasi added this pull request to the merge queue Oct 5, 2023
Merged via the queue into main with commit 3259da0 Oct 5, 2023
@liamdebeasi liamdebeasi deleted the FW-4403 branch October 5, 2023 18:45
github-merge-queue bot pushed a commit that referenced this pull request Mar 15, 2024
Issue number: resolves #28867 --------- <!-- Please do not submit updates to dependencies unless it fixes an issue. --> <!-- Please try to limit your pull request to one type (bugfix, feature, etc). Submit multiple pull requests if needed. --> ## What is the current behavior? <!-- Please describe the current behavior that you are modifying. --> I missed an edge case in #28277 that caused an MD headers in an MD app to be hidden due to the presence of an iOS header (via `mode="ios"`). This was happening because I forgot to scope the selector in `header.ios.scss` to only iOS headers. ## What is the new behavior? <!-- Please describe the behavior or changes that are being added by this PR. --> - The headers in the relevant selector are now scoped to the iOS headers which avoids this bug while preserving the anti-flicker mechanism added in the linked PR. ## Does this introduce a breaking change? - [ ] Yes - [x] No <!-- If this introduces a breaking change: 1. Describe the impact and migration path for existing applications below. 2. Update the BREAKING.md file with the breaking change. 3. Add "BREAKING CHANGE: [...]" to the commit description when merging. See https://github.com/ionic-team/ionic-framework/blob/main/.github/CONTRIBUTING.md#footer for more information. --> ## Other information <!-- Any other information that is important to this PR such as screenshots of how the component looks before and after the change. --> Dev build: `7.8.1-dev.11710452743.1ca99e5e` --------- Co-authored-by: ionitron <hi@ionicframework.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

package: core @ionic/core package

2 participants