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(ios): large title transition accounts for back button with no text #29327

Merged
merged 9 commits into from Apr 30, 2024

Conversation

liamdebeasi
Copy link
Contributor

@liamdebeasi liamdebeasi commented Apr 11, 2024

Issue number: resolves #28751


What is the current behavior?

The large title transition does not account for back buttons with no text value. We assume that the .button-text element is always defined, but that is not the case when text="" on the back button. As a result, devs were getting errors because we tried to get the bounding box of a undefined.

What is the new behavior?

  • Revised the large title logic to only grab values from the back button text if the back button text element is actually defined

There should be no behavior change when the back button text element is defined.

Does this introduce a breaking change?

  • Yes
  • No

Other information

Dev Build: 7.8.5-dev.11713282122.18cabf98

⚠️ Reviewers: Please test this in the sample application on the linked issue. Please be sure to test the following conditions:

  1. When the back button text is defined
  2. When the back button text is not defined
  3. With the default font scale
  4. With a larger font scale

Copy link

vercel bot commented Apr 11, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
ionic-framework ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 16, 2024 3:44pm

@liamdebeasi liamdebeasi changed the title Fw 5818 fix(iOS): large title transition accounts for back button with no text Apr 16, 2024
@liamdebeasi liamdebeasi changed the title fix(iOS): large title transition accounts for back button with no text fix(ios): large title transition accounts for back button with no text Apr 16, 2024
@sean-perkins
Copy link
Contributor

Observing a shift to both the button and the large title when popping the view. Doesn't happen consistently, but seems to be related to adjusting the text size.

Could be a Safari quirk if you aren't able to reliable reproduce. I can get Safari's header UI to break/run into the content, so there could be more factors here.

RPReplay_Final1713460282.MP4

@liamdebeasi
Copy link
Contributor Author

If you set text=" " on the BackButton (note the space) based off main, does the issue reproduce?

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, worked on the original repo and on a simulator

Copy link
Contributor

@sean-perkins sean-perkins left a comment

Choose a reason for hiding this comment

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

Tested against the latest changes of v7 with text=" " and was unable to reproduce the video I attached earlier.

Tested again with the dev-build and was only able to get in that state once with changing the dynamic font scaling, but was unable to reproduce with the same steps consistently. Navigated 40+ times without the issue presenting itself with a consistent dynamic font scale.

I think these changes are good to merge. There is a rendering quirk in Safari's UI with changing the dynamic font scale with the address bar at the top of the screen that is having other impact (at random, but not very common). If we get issue reports we can do a deeper evaluation of what is going on there.

@liamdebeasi liamdebeasi added this pull request to the merge queue Apr 30, 2024
Merged via the queue into main with commit bd8d065 Apr 30, 2024
58 checks passed
@liamdebeasi liamdebeasi deleted the FW-5818 branch April 30, 2024 15:31
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.

bug: large title transition does not account for empty back button text
3 participants