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

PLANET-7647 Enforce consistency on default favicon #2507

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mleray
Copy link
Contributor

@mleray mleray commented Jan 21, 2025

Description

See PLANET-7647

We remove the option to customize it in the site identity settings, and we need to run a migration to remove existing values

Testing

1. Setting removal

If you go to Appearance > Customize > Site Identity you should no longer be able to change the site icon.

Before the removal you would see this:

Screenshot 2025-01-21 at 10 37 33

2. Migration script

You can run it on local with npx wp-env run cli wp p4-run-activator

Before the migration, we had a custom site icon:

Screenshot 2025-01-21 at 11 09 24

After the migration, you should now see the default site icon:

Screenshot 2025-01-21 at 11 09 33

@mleray mleray self-assigned this Jan 21, 2025
planet-4 added a commit to greenpeace/planet4-test-titan that referenced this pull request Jan 21, 2025
/unhold db2c7bb6-ff3e-4724-bce8-a489944395ff
planet-4 added a commit to greenpeace/planet4-test-titan that referenced this pull request Jan 21, 2025
/unhold dd69ef98-2642-443c-8125-ee64fb0a81a9
We remove the option to customize it in the site identity settings, and we need to run a migration to remove existing values
planet-4 added a commit to greenpeace/planet4-test-titan that referenced this pull request Jan 21, 2025
/unhold e137cd9c-7d5a-457a-94bf-e3ae73c7fe66
@mleray mleray added the Review label Jan 21, 2025
planet-4 added a commit to greenpeace/planet4-test-titan that referenced this pull request Jan 21, 2025
/unhold 4fd16a74-3aac-4022-b8b9-f491e69bbdb9
@greenpeace greenpeace deleted a comment from planet-4 Jan 21, 2025
planet-4 added a commit to greenpeace/planet4-test-tavros that referenced this pull request Jan 21, 2025
/unhold 987691d2-91db-4c08-a6e0-2aca9c68f5c1
@planet-4
Copy link
Contributor

Test instance is ready 🚀

🌑 tavros | admin | blocks report | CircleCI | composer-local.json

⌚ 2025.01.21 11:26:41

@mleray mleray marked this pull request as ready for review January 21, 2025 12:06
@mleray mleray requested review from comzeradd, a team and Osong-Michael and removed request for a team January 21, 2025 12:06
Copy link
Contributor

@Osong-Michael Osong-Michael left a comment

Choose a reason for hiding this comment

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

Hey @mleray after the migration, we loose the favicon for the page when on the admin dashboard and not the site itself, is this the expected behaviour?

Test Instance
Screenshot 2025-01-21 at 13 47 48

Local Instance
Screenshot 2025-01-21 at 13 48 44

PS: Does not seem like a big deal since only editors use this.

@mleray
Copy link
Contributor Author

mleray commented Jan 21, 2025

Hey @mleray after the migration, we loose the favicon for the page when on the admin dashboard and not the site itself, is this the expected behaviour?

@Osong-Michael I do not see this on the instance 🤔 The icon is updated in the admin interface too

Screenshot 2025-01-21 at 14 49 27

Edit: I do see the issue on local, but only in certain admin pages, not all of them... weird!

@mleray mleray requested a review from Osong-Michael January 21, 2025 16:09
Copy link
Contributor

@Osong-Michael Osong-Michael left a comment

Choose a reason for hiding this comment

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

Code looks good to me 👍

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

Successfully merging this pull request may close these issues.

3 participants