WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 years ago

Last modified 16 months ago

#15228 closed defect (bug) (fixed)

Hello Dolly can't cope with the admin bar

Reported by: nacin Owned by: kapeels
Milestone: 3.1 Priority: highest omg bbq
Severity: normal Version: 3.1
Component: Administration Keywords: has-patch dev-feedback
Focuses: Cc:

Description

And given the choice to deactivate Hello Dolly or turn off the admin bar, I had no choice but to turn off the admin bar.

Screenshot attached.

Attachments (2)

dolly.png (18.2 KB) - added by nacin 3 years ago.
Not looking so swell here :-(
hello.diff (759 bytes) - added by kapeels 3 years ago.
Patch.

Download all attachments as: .zip

Change History (15)

nacin3 years ago

Not looking so swell here :-(

comment:1 jane3 years ago

  • Keywords needs-patch added

comment:2 kapeels3 years ago

  • Cc kapeels added
  • Owner set to kapeels
  • Status changed from new to accepted

comment:3 kapeels3 years ago

  • Keywords 2nd-opinion has-patch needs-testing added; needs-patch removed

Added jQuery code to check the location of #wp-head and to change the position of #dolly accordingly.

Patch - hello.diff

PS - This is my first patch. Please point mistakes. Thanks.

comment:4 sbressler3 years ago

  • Cc sbressler@… added

This is just about the most amusing bug I have seen :)

Using JS to position the text is too heavy-weight for a plugin that's meant to showcase the simplicity of writing a plugin. Absolute position in general is lackluster. Either remove the positioning and just attach the output to an appropriate admin filter or explicitly check for the existence of the admin bar.

comment:5 follow-up: kapeels3 years ago

  • Keywords needs-codex added; needs-testing removed

Okay. So here's a new one, this uses admin_notices action and works good. But there doesn't seems to be a documentation for admin_notices. Could anybody add that?

Attaching hello.diff

kapeels3 years ago

Patch.

comment:6 kapeels3 years ago

  • Keywords dev-feedback added; 2nd-opinion removed

comment:7 nacin3 years ago

This looks good.

comment:8 nacin3 years ago

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

(In [16270]) Hello Dolly 1.6. fixes typo, props sbressler, fixes #15229. Removes absolute from positioning, props kapeels, fixes #15228. Add some whitespace.

comment:9 nacin3 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

admin_notices is only for the blog admin. We'll need to use all_admin_notices here.

That's annoying however, as linking both will mean that you'll get it twice, and adding all three (network, user) doesn't scale if we ever come up with a fourth. I've proposed a change to the functionality in #14696. If that goes through, this can be re-closed as fixed.

comment:10 nacin3 years ago

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

comment:11 follow-up: kapeels3 years ago

@nacin Does that really mean I fixed my first bug in WordPress? :D

As @sbressler said, this plugin showcases simplicity of writing a plugin. I feel there should be some codex docs for the actions.

comment:12 in reply to: ↑ 11 nacin3 years ago

Replying to kapeels:

@nacin Does that really mean I fixed my first bug in WordPress? :D

Yes! Congratulations. :)

As @sbressler said, this plugin showcases simplicity of writing a plugin. I feel there should be some codex docs for the actions.

It would be cool to eventually add some more inline documentation and comments, a la Twenty Ten.

comment:13 in reply to: ↑ 5 SergeyBiryukov16 months ago

  • Keywords needs-codex removed

Replying to kapeels:

But there doesn't seems to be a documentation for admin_notices. Could anybody add that?

http://codex.wordpress.org/Plugin_API/Action_Reference/admin_notices

Note: See TracTickets for help on using tickets.