WordPress.org

Make WordPress Core

Opened 8 years ago

Closed 8 years ago

Last modified 6 years 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 8 years ago.
Not looking so swell here :-(
hello.diff (759 bytes) - added by kapeels 8 years ago.
Patch.

Download all attachments as: .zip

Change History (15)

@nacin
8 years ago

Not looking so swell here :-(

#1 @jane
8 years ago

  • Keywords needs-patch added

#2 @kapeels
8 years ago

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

#3 @kapeels
8 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.

#4 @sbressler
8 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.

#5 follow-up: @kapeels
8 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

@kapeels
8 years ago

Patch.

#6 @kapeels
8 years ago

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

#7 @nacin
8 years ago

This looks good.

#8 @nacin
8 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.

#9 @nacin
8 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.

#10 @nacin
8 years ago

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

#11 follow-up: @kapeels
8 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.

#12 in reply to: ↑ 11 @nacin
8 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.

#13 in reply to: ↑ 5 @SergeyBiryukov
6 years 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.