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(searchbar): autocapitalize is initialized correctly #29197

Merged
merged 1 commit into from Mar 21, 2024
Merged

Conversation

liamdebeasi
Copy link
Member

@liamdebeasi liamdebeasi commented Mar 21, 2024

Issue number: resolves #29193


What is the current behavior?

In an attempt to preserve backwards compatibility when adding autocapitalize to Searchbar, we used ! to indicate that the prop was never undefined. The autocapitalize on HTMLElement expects this value to be a string and never undefined.

For the purposes of the property on Searchbar, setting this prop to one of the accepted values would constitute a breaking change because it would override the default browser behavior (which we previously relied upon). As a result, we used ! to not set a default prop but inform TypeScript that this prop is always defined. This unintentionally made it so developers needed to define the autocapitalize property every time which is not what we want.

What is the new behavior?

  • autocapitalize now defaults to the 'default' keyword. This is an internal keyword that is used to tell Ionic to not set the autocapitalize attribute on the inner input element and instead rely on the default browser behavior. This satisfies the HTMLElement requirement that autocapitalize is never undefined. In Ionic 8 this 'default' value will be replaced with 'off'.

Typescript currently sets the HTMLElement autocapitalize type to string which is why we can add a 'default' keyword here. There is some risk that if these type definitions change in the future that applications may encounter errors. However, changing this on the TypeScript side would itself be a breaking change. Additionally, we are moving away from 'default' in Ionic 8, so I think this is an acceptable amount of risk.

Does this introduce a breaking change?

  • Yes
  • No

Other information

Dev build: 7.8.2-dev.11711027016.13b2a920

Tested in a React starter app with Searchbar, and I verified this fix works.

@github-actions github-actions bot added the package: core @ionic/core package label Mar 21, 2024
@liamdebeasi liamdebeasi marked this pull request as ready for review March 21, 2024 13:27
@liamdebeasi liamdebeasi requested review from brandyscarney and a team as code owners March 21, 2024 13:27
Copy link
Contributor

@mapsandapps mapsandapps left a comment

Choose a reason for hiding this comment

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

Makes sense. I tested it & it works 👍🏻

@liamdebeasi liamdebeasi added this pull request to the merge queue Mar 21, 2024
Merged via the queue into main with commit 8ad66c0 Mar 21, 2024
56 checks passed
@liamdebeasi liamdebeasi deleted the FW-6055 branch March 21, 2024 17:00
aeharding added a commit to aeharding/voyager that referenced this pull request Mar 22, 2024
aeharding added a commit to aeharding/voyager that referenced this pull request Mar 22, 2024
* Upgrade dependencies
* ionic v8 upgrade
* Remove old browser support (due to Ionic upgrade) incl. iOS 14 support
* Temporary patch for Ionic issue ionic-team/ionic-framework#29197
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: autocapitalize is required in IonSearchbar types
3 participants