WordPress.org

Make WordPress Core

Opened 5 months ago

Closed 4 months ago

Last modified 4 months ago

#26119 closed enhancement (fixed)

New admin styling: Remove screen_icon() calls

Reported by: TobiasBg Owned by: nacin
Milestone: 3.8 Priority: highest omg bbq
Severity: minor Version: 3.8
Component: Administration Keywords: has-patch
Focuses: ui Cc:

Description

The new admin styling ("TSFKAMP6" -- The styling formerly known as MP6.) does not show the screen icon next to each screen's <h2> heading anymore. They are hidden via CSS in wp-admin.css:

.icon32 {
	display: none;
}

This is necessary to also hide those icons on admin screens that are added by plugins.

Additionally, to remove some unnecessary HTML code, we should remove all calls to screen_icon() from Core's admin screens.

We could then even consider deprecating screen_icon() and get_screen_icon().

Attachments (3)

26119-remove-screen_icon.patch (26.0 KB) - added by TobiasBg 5 months ago.
Remove screen_icon() calls
26119-deprecate-functions.patch (1.2 KB) - added by TobiasBg 5 months ago.
Deprecate screen icon functions
26199-deprecate-move-functions.patch (3.5 KB) - added by TobiasBg 5 months ago.
Deprecate and move screen icon functions

Download all attachments as: .zip

Change History (27)

TobiasBg5 months ago

Remove screen_icon() calls

TobiasBg5 months ago

Deprecate screen icon functions

comment:1 nacin5 months ago

  • Milestone changed from Awaiting Review to 3.8

A deprecated function can always be un-deprecated, but once we do this, it's going to be tougher to go back. Might we ever want icons again?

comment:2 SergeyBiryukov5 months ago

  • Keywords ui-focus has-patch added

comment:3 TobiasBg5 months ago

26119-deprecate-functions.patch deprecates the two functions but does not move them to deprecated.php, as I'm having trouble right now with figuring out how to move just the functions using git svn while preserving history. Sorry.

comment:4 nacin5 months ago

Neither SVN nor Git preserve history across files. Git tries to "fake" it with git blame -C, that's about it though.

TobiasBg5 months ago

Deprecate and move screen icon functions

comment:5 TobiasBg5 months ago

Ah, ok. Thought that I had seen it with preserved history somewhere. Thanks!
New patch 26199-deprecate-move-functions.patch​ also moves the files to deprecated.php.

comment:6 GaryJ5 months ago

  • Cc gary@… added

comment:7 jdgrimes5 months ago

  • Cc jdg@… added

comment:8 matt5 months ago

  • Priority changed from normal to highest omg bbq
  • Severity changed from normal to minor

comment:9 azaozz5 months ago

  • Owner set to azaozz
  • Resolution set to fixed
  • Status changed from new to closed

In 26518:

Remove all screen_icon() calls and deprecate the functions, props TobiasBg, fixes #26119

comment:10 GaryJ4 months ago

It's disappointing to see the functions formally deprecated less than two weeks before 3.8 final is expected to drop. Plugins and [child] themes using it now only have that time to get a new release out if they want to avoid deprecation notices appearing in 3.8 with WP_DEBUG enabled.

I think the formal deprecation could have waited until 3.9, even if WP core wasn't using it in the meantime.

comment:11 follow-up: TobiasBg4 months ago

But how many users have WP_DEBUG enabled AND are using a plugin that brings its own admin screens, including screen_icon() calls? There's far more plugins that are doing_it_wrong() and that create notices with the enqueue_script hooks or even $wpdb->prepare().
Also, not many plugin devs have already released their plugin's "3.8 compatibility release", I would think. And even then: This is not causing backward compatibility problems, it's just resulting in a notice for a small portion of users.

comment:12 in reply to: ↑ 11 ; follow-up: toscho4 months ago

  • Cc info@… added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Replying to TobiasBg:

But how many users have WP_DEBUG enabled AND are using a plugin that brings its own admin screens, including screen_icon() calls?

Nobody knows.

Also, not many plugin devs have already released their plugin's "3.8 compatibility release", I would think.

Sorry, this is wrong. Especially in commercial plugins with developer packs, we cannot deliver plugins that are raising notices, and we cannot update all of them in such a short time.

Please wait for the next version with the depreciation. That does really not hurt anyone.

comment:13 Bueltge4 months ago

  • Cc frank@… added

comment:14 blepoxp4 months ago

  • Cc glenn@… added

comment:15 markoheijnen4 months ago

I completely missed this. What is the reason why the icons are gone?

I do think that this function will be used for a while. Even if it will show a notice because not everyone will update their WordPress install but they will update plugins. So you do want to have the same look and feel for exaple 3.6 and 3.7.

comment:16 in reply to: ↑ 12 chrisbliss184 months ago

I second the idea of delaying the deprecation until the next release cycle. While simply ripping out the function calls is easy enough, we'll likely have to add a version check to call the function for pre-3.8 versions so that people updating our plugins/themes before 3.8 launches (and for those that will wait a few weeks before updating, because many do) have admin pages that are consistent with the rest of the UI.

I believe that we can update all of our code in time for the 3.8 launch, but what about smaller teams, developers that maintain their code on their off time, and people that won't know about this deprecation until it is possibly too late to be able to update everything?

It's easy to dismiss the concern as trivial as WP_DEBUG has to be enabled, but from experience, many non-developers have this turned on and see any messages that come up as a big concern that needs to be fixed immediately. I can't simply tell paying customers that their concern is of no concern to us.

comment:17 layotte4 months ago

  • Cc lew@… added

comment:18 nacin4 months ago

  • Owner changed from azaozz to nacin
  • Status changed from reopened to accepted

The original screen icons were rich with detail and worked well in WordPress 2.7. In 3.2, when things got a bit more streamlined and felt more like an "app", they seemed somewhat out of place. In 3.8, the new design flattens and simplifies things. With everything being an icon font, a 32-pixel version would provide no more detail than a 20-pixel version used in the menus.

I honestly have zero qualms with deprecating these functions. I'm sorry that paying customers have WP_DEBUG turned on, but we don't lightly deprecate things (say, renaming for the sake of renaming). Deprecation isn't just about "running clean" but about alerting developers to changes in functionality. I'm OK with un-deprecating them since this happened late in the cycle, but the first time we get a support thread or ticket asking why screen_icon() isn't printing anything anymore, I'm going to march over to this ticket and casually drop a link. :-)

I think a good approach here would be to keep the functions (un-deprecated) but have them return/echo nothing. We could probably remove the CSS too, at that point.

comment:19 chrisbliss184 months ago

Sound like a good compromise to me.

I appreciate that there really isn't a perfect solution to this one that doesn't result in confusion and complaints from some group of people. If it gives you any type of humor or relief when responding to such complaints, I fully support the idea of responding to questions about the missing output with "because Chris Jean is a jerk and likes to complain," "#blame chrisbliss18," or something to that effect. :)

comment:20 follow-up: nacin4 months ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 26537:

Only informally deprecate get_screen_icon() and screen_icon(). fixes #26119.

comment:21 in reply to: ↑ 20 ; follow-up: ericlewis4 months ago

Replying to nacin:

In 26537

What does /**#@+ mean?

comment:22 in reply to: ↑ 21 GaryJ4 months ago

Replying to ericlewis:

Replying to nacin:

In 26537

What does /**#@+ mean?

It means that the DocBlock applies for all structural elements (here, functions) until the closing template tag. WP only has 5 existing instances of DocBlock Templates (1 in SimplePie). See this for more info - it's usually used for self-documenting class properties.

It's also wrong here, since it now appears that get_screen_icon() @uses get_screen_icon().

comment:23 nacin4 months ago

In 26541:

Remove @uses from get_screen_icon() / screen_icon(). see #26119.

comment:24 F J Kaiser4 months ago

I'm not a big fan of deprecating those functions. Icons are used to help users with recognizing things quicker than they could by reading. (I always hoped we could use them as page specific admin favicons if no other one was present).

If core doesn't use it anymore: ok. But we should not completely remove the functionality for everyone. At least not during a Beta cycle...

Note: See TracTickets for help on using tickets.