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.

Sign up for GitHub

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

Jump to bottom

bring back support for favicon.ico #20612

Merged
merged 8 commits into from
Jan 16, 2023
Merged

bring back support for favicon.ico #20612

merged 8 commits into from
Jan 16, 2023

Conversation

ndelangen
Copy link
Member

Issue: #20487

What I did

  • resolve the favicon path in a preset-property
  • pick the first one, if any is found, warn user, if we found multiple
  • make the link tag injection work for both svg and ico

@ndelangen ndelangen self-assigned this Jan 13, 2023
@ndelangen ndelangen added the bug label Jan 13, 2023
@ndelangen ndelangen changed the title add support for favicon.ico again bring back support for favicon.ico Jan 13, 2023
@ndelangen ndelangen requested a review from shilman January 13, 2023 16:16
@ndelangen
Copy link
Member Author

I'd like to give this a manual test before merging

@ndelangen ndelangen requested review from tmeasday and removed request for shilman January 14, 2023 00:29
shilman
shilman requested changes Jan 15, 2023
View reviewed changes
Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

Generally LGTM!

code/lib/core-server/src/presets/common-preset.ts Outdated Show resolved Hide resolved
code/lib/core-server/src/presets/common-preset.ts Outdated Show resolved Hide resolved
@ndelangen ndelangen requested a review from shilman January 16, 2023 15:12
@ndelangen ndelangen merged commit a2080b9 into next Jan 16, 2023
@ndelangen ndelangen deleted the norbert/bring-favicon-ico-support-back branch January 16, 2023 15:19
@shilman shilman mentioned this pull request Jan 17, 2023
Closed
@renato-bohler renato-bohler mentioned this pull request Feb 24, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@valentinpalkovic valentinpalkovic valentinpalkovic approved these changes

@tmeasday tmeasday Awaiting requested review from tmeasday

@shilman shilman Awaiting requested review from shilman

Assignees

@ndelangen ndelangen

Labels
bug
Projects
Core Team Projects
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ndelangen @shilman @valentinpalkovic