Site Icon: Start using core's Site Icon by dereksmart · Pull Request #2402 · Automattic/jetpack

dereksmart · 2015-07-14T21:50:23Z

This is meant to be a seamless under-the-hood transition to deprecating Jetpack's site icon in favor of core.

If user already has Jetpack Site Icon set, this will take that icon, transfer it to core's version, and deactivate Jetpack's Site Icon.
Modifies the Module list on the Jetpack Settings page so Site Icon cannot be activated. Instead, we tell them about Core's new Site Icon and a link to configure it.
Makes sure that core's site icon also gets synced back to wpcom

My thinking is that we'll leave it like this until 4.3 is the minimum supported WP version. Then we can remove the module completely.

This includes @kraftbj's commits from #2328 as well, so that PR can be discarded.

fixes #2326

roccotripaldi · 2015-07-14T22:18:29Z

class.jetpack-modules-list-table.php

Maybe instead of using an id, we could use a more generic class, like 'module-deprecated' so that we can use it for other deprecated modules.

The only real purpose of the ID in this case is a target for the genericon, so making this reusable wouldn't really work for other modules.

Ah. Thanks for the clarification.

enejb · 2015-07-15T22:16:48Z

It doesn't seem to be working as expected.

See:

I am expecting to not see the icon any more. The site icon module is still active, I think we should disable it and only expose.

We should also depreciate the jetpack_site_icon_url and jetpack_get_site_icon and jetpack_has_site_icon functions in favour of the new WP 3.6 functions.

enejb · 2015-07-15T22:22:49Z

class.jetpack.php

I think this part doesn't make so much sense to me.
Since we should just not load the module anymore if the user is running 3.6 not just if the user has an site icon or not.

And this part needs to be tested from both ways. If the user is running jetpack WP 4.2 and then upgrade to WP 4.3 and the latest jetpack or if the user is on jetpack 3.5 and upgrades to jetpack 3.6 and is running WP 4.3.

If you continue reading this function, the module should be disabled whether or not there is a core icon set. I did it here early bc if one is already set then the rest of the code is unnecessary overhead. This function runs on admin_init, so I'm curious how you're experiencing the module not deactivating in your comment above.

If upgrading from WP 4.2 -> 4.3, this function will run on admin_init, transfer the icon to core then deactivate the module. Or if no icon is set, it will just deactivate the module.

Same with upgrading from JP 3.5 -> 3.6.1

zinigor · 2015-07-16T11:53:46Z

Read the code, tested the transition between 4.2.2 and trunk, everything works as expected.

enejb · 2015-07-16T16:30:38Z

So it turned out that not all the code was update on the servers. This PR fixes the issue that I reported earlier about seeing 2 Site Icon forms.

enejb · 2015-07-16T22:53:25Z

We need to make sure that when the user updates the site icon in 4.3 that we sync the data .com right away not just on the heartbeat.

zinigor · 2015-07-17T06:57:58Z

@enej doesn't that do the trick? https://github.com/Automattic/jetpack/pull/2402/files#diff-9ba9fa5bb52bd6cf36c6c0bdd5ed4830R594

dereksmart · 2015-07-17T11:32:38Z

@enejb the hook that Igor pointed out does the trick. It's using add_option instead of update_option because Core's process for uploading a site icon is to first delete the option entirely, then add a new one.

enejb · 2015-07-17T16:03:12Z

@zinigor and @dereksmart I think we might also need do something like:
add_action( "delete_option_site_icon", , array( $this, 'jetpack_sync_core_icon' ) ); add_action( "update_option_site_icon", , array( $this, 'jetpack_sync_core_icon' ) ); add_action( "add_option_site_icon", , array( $this, 'jetpack_sync_core_icon' ) );
to capture all the cases. :)

dereksmart · 2015-07-17T16:05:48Z

delete_option_site_icon is a good idea. I first tried update_option_site_icon instead of add, but it wasn't working for me. I think it's because the process of adding/updating an icon in core is to first delete, then update the option (thus adding a new option), therefore the update_option hook never fires.

Guess it wouldn't hurt to add though.

obenland · 2015-07-17T17:34:01Z

class.jetpack.php

Is there a better hook than admin_init? Or a better place to call it, like in Jetpack_Site_icon itself?
Currently it would add that callback for every admin page load for(almost)ever.

jeffgolenski · 2015-07-17T19:57:37Z

Alright I tided up some markup and made the deprecated setting look decent / functional on mobile devices. I moved the checkbox back in and just disabled it to maintain visual cohesion. Disable is pretty well supported all around.

Also important to note. I made "Configure" capitalized to match the others, just made that change after the screenshots were taken.

…used.

… WP.

New function will return true if core's site icon is available. It will also convert Jetpack's site icon to Core's version if one is set. It will disable Jetpack's site icon module no matter what.

Don't allow activation and point config link to core's site icon

We need to sync core's site icon to use in wpcom. This will allow the icon to be synced when it's uploaded to core. Also run a check on `jetpack_heartbeat` action in case core icon was set before Jetpack was installed

…on` (ID) to sync to dotcom as well.

Cleaned up a few things for the newly deprecated Site Icon. Also made it look decent / function on mobile devices.

No need to run this on every admin page.

Site Icon: Begin the deprecation of Jetpack's Site Icon in favor of Core

dereksmart · 2015-07-19T15:48:50Z

Keeping this branch open in case there's any more discussion. Will delete after 3.6.1 is released.

dereksmart added [Feature] Site Icon [Status] Needs Review This PR is ready for review. labels Jul 14, 2015

dereksmart added this to the 3.6.1 milestone Jul 14, 2015

dereksmart added the [Status] In Progress label Jul 14, 2015

roccotripaldi reviewed Jul 14, 2015
View reviewed changes

dereksmart added [Status] Requires String Changes and removed [Status] In Progress labels Jul 15, 2015

dereksmart force-pushed the fix/site-icon-use-core branch from d2427c6 to 4b401a7 Compare July 15, 2015 17:28

enejb reviewed Jul 15, 2015
View reviewed changes

zinigor added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review This PR is ready for review. labels Jul 16, 2015

dereksmart mentioned this pull request Jul 17, 2015

My Jetpack is missing content. #2411

Closed

dereksmart force-pushed the fix/site-icon-use-core branch from 12eb908 to 14e9ebd Compare July 17, 2015 17:17

obenland reviewed Jul 17, 2015
View reviewed changes

dereksmart force-pushed the fix/site-icon-use-core branch 3 times, most recently from f71dc45 to 7e52cc3 Compare July 17, 2015 18:08

jeffgolenski added the [Status] Design Review Complete label Jul 17, 2015

zinigor force-pushed the fix/site-icon-use-core branch from 2031347 to 29a9b8a Compare July 17, 2015 21:32

kraftbj and others added 12 commits July 17, 2015 17:50

Open Graph: Adds fallback to Core's site icon. See #2326

d634313

Site Icon: Kill output of icons if Core's Site Icon feature is being …

8d350ea

…used.

Site Icon: Adjust Core conditional logic to fire on older versions of…

9fc8497

… WP.

Site Icon: Check if core has site icon, and if so use it

01cd35d

New function will return true if core's site icon is available. It will also convert Jetpack's site icon to Core's version if one is set. It will disable Jetpack's site icon module no matter what.

Customize the module list for Site Icon

fb1497d

Don't allow activation and point config link to core's site icon

don't allow link to modal for custom deprecated site icon

f6c21df

Site Icon: change enqueue slug so it doesn't clash with core's

0fa5181

Better checks to aviod errors

f3aa3cf

Site Icon: Make sure Core's Icon is syncable as well

eabedf6

We need to sync core's site icon to use in wpcom. This will allow the icon to be synced when it's uploaded to core. Also run a check on `jetpack_heartbeat` action in case core icon was set before Jetpack was installed

Core Site Icon: trigger sync on all site_icon actions. Add `site_ic…

fa72b63

…on` (ID) to sync to dotcom as well.

Made deprecated site icon mobile friendly

31fdb62

Cleaned up a few things for the newly deprecated Site Icon. Also made it look decent / function on mobile devices.

Make string translatable

f5b5525

dereksmart force-pushed the fix/site-icon-use-core branch from 29a9b8a to f5b5525 Compare July 17, 2015 21:55

Site Icon: Only call code when needed.

cd84798

No need to run this on every admin page.

dereksmart added a commit that referenced this pull request Jul 19, 2015

Merge pull request #2402 from Automattic/fix/site-icon-use-core

db913ea

Site Icon: Begin the deprecation of Jetpack's Site Icon in favor of Core

dereksmart merged commit db913ea into master Jul 19, 2015

dereksmart removed the [Status] Ready to Merge Go ahead, you can push that green button! label Jul 19, 2015

dereksmart deleted the fix/site-icon-use-core branch July 19, 2015 15:46

dereksmart restored the fix/site-icon-use-core branch July 19, 2015 15:47

kraftbj deleted the fix/site-icon-use-core branch August 5, 2015 21:34

aduth mentioned this pull request Jan 2, 2017

SAL: Move icon logic to Jetpack base, leverage core site icon option consistently #6025

Merged

Conversation

dereksmart commented Jul 14, 2015

Uh oh!

roccotripaldi Jul 14, 2015

Choose a reason for hiding this comment

Uh oh!

dereksmart Jul 14, 2015

Choose a reason for hiding this comment

Uh oh!

roccotripaldi Jul 14, 2015

Choose a reason for hiding this comment

Uh oh!

enejb commented Jul 15, 2015

Uh oh!

enejb Jul 15, 2015

Choose a reason for hiding this comment

Uh oh!

dereksmart Jul 15, 2015

Choose a reason for hiding this comment

Uh oh!

zinigor commented Jul 16, 2015

Uh oh!

enejb commented Jul 16, 2015

Uh oh!

enejb commented Jul 16, 2015

Uh oh!

zinigor commented Jul 17, 2015

Uh oh!

dereksmart commented Jul 17, 2015

Uh oh!

enejb commented Jul 17, 2015

Uh oh!

dereksmart commented Jul 17, 2015

Uh oh!

obenland Jul 17, 2015

Choose a reason for hiding this comment

Uh oh!

jeffgolenski commented Jul 17, 2015

Uh oh!

dereksmart commented Jul 19, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants