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(icon): add better warning when loading icons #1297

Merged
merged 3 commits into from
Nov 6, 2023
Merged

Conversation

liamdebeasi
Copy link
Contributor

@liamdebeasi liamdebeasi commented Nov 1, 2023

In ionic-team/ionic-framework#28445 we received feedback that accidentally referring to an icon by name without first registering it is confusing because devs get a cryptic "invalid URL" error. This PR attempts to improve that experience by providing a more helpful message and giving developers steps they can take to fix the issue.

main branch
image image

One note: This warning will be logged every time the icons tries to load. We currently load the icon in connectedCallback and componentDidLoad, so this warning can be logged twice on page load. In Angular, one more warning may be logged if the icon is in a router outlet (since the elements are moved into ion-router-outlet on load which means they are first removed from the DOM, so connectedCallback fires twice.). We could add a cache that tracks if we already warned about each icon per element instance if reviewers prefer (though that will slightly increase the complexity of this fix).

@mapsandapps mapsandapps requested review from a team and averyjohnston and removed request for mapsandapps and a team November 1, 2023 14:19
Copy link
Contributor

@averyjohnston averyjohnston left a comment

Choose a reason for hiding this comment

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

I'm okay with the fix as-is; I would rather the warning log too many times than open the risk of a bug in the cache causing it to not log when it needs to.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants