Collapse additional avatar settings if avatars are disabled

[valendesigns]

Loads the JavaScript via the admin_print_footer_scripts action instead of inline.

[valendesigns]

Hey Guys,

When I added my updated patch I didn't know trac wouldn't send you a notification. I'm commenting on this ticket now so you know I made a couple changes to the way the JavaScript is added to the page.

Cheers,
Derek

[valendesigns]

I just updated the patch once more. This time to decouple the JavaScript from the markup a bit, so it's future proof when/if the markup changes and we no longer use tables. And changed the naming of variables and CSS classes in the process since we wouldn't be using the tr anymore to target elements.

[helen]

Was tweaking a couple things, looking to commit (see 30168.5.diff​), but noticed something that's a little funky - when avatars are off, the previews for generated avatars don't show. Seems understandable, especially when trying not to hit any external services, but it jumped out to me once I was toggling the section "on". Curious if we think it's worth addressing, or if it's really separate.

[valendesigns]

Replying to helen:

Was tweaking a couple things, looking to commit (see 30168.5.diff​), but noticed something that's a little funky - when avatars are off, the previews for generated avatars don't show. Seems understandable, especially when trying not to hit any external services, but it jumped out to me once I was toggling the section "on". Curious if we think it's worth addressing, or if it's really separate.

I don't think this will stop the external hit even though they are hidden. However, I feel like the point of the patch is to hide all avatar settings and should be the expected behavior when show_avatars is not checked. It probably feels funky because we're so use to seeing those generated avatars when you visit the Discussion page. Just my 2¢.

I'm glad to see this is going to make it into 4.2 though!

[nacin]

I don't think we should try to do this to avoid external services hits. That could be better achieved with a filter on default_avatar_select and/or avatar_defaults and/or in get_avatar() itself. Patch looks good, though perhaps we should simplify it so a 'hidden' class is included with 'avatar-settings', and then all we do is $('.avatar-settings').toggleClass('hidden'). That also means there definitely won't be any noticeable jumps during load, though in practice this might be tough to notice.

[helen]

Ah, sorry, I meant that there are no avatar previews when you toggle the setting and subsequent sections on when previously avatars were off. We've got a .hidden class, can go with that.

[valendesigns]

Replying to helen:

Ah, sorry, I meant that there are no avatar previews when you toggle the setting and subsequent sections on when previously avatars were off. We've got a .hidden class, can go with that.

Sorry, I see what you mean now. I completely misunderstood you. In order to make those show up we would need to remove the first 2 lines from get_avatar.

if ( ! get_option('show_avatars') )
	return false;

[valendesigns]

I refreshed the patch to toggle the .hidden class. Hopefully, it's what you had in mind @nacin.

[iseulde]

I really don't like inline JavaScript. One reason is that it's hidden from the eyes of JSHint. Not that it's super important for this much code, but before you know it a few lines become many.

What about merging this to a file with the other inline Javascript blocks on the other options pages? Reading, Permalinks and General has some.

[helen]

Replying to avryl:

I really don't like inline JavaScript. One reason is that it's hidden from the eyes of JSHint. Not that it's super important for this much code, but before you know it a few lines become many.

What about merging this to a file with the other inline Javascript blocks on the other options pages? Reading, Permalinks and General has some.

Probably fine, but in a separate ticket. Not going to hold this up over that. :)

[iseulde]

Ok, just adjusted the patch a bit. :)

[helen]

Replying to valendesigns:

In order to make those show up we would need to remove the first 2 lines from get_avatar.

We can use pre_option_show_avatars to force it for just this instance, see 30168.7.diff​.

Using an existing class allows us to set it on the server side so there's no jumping on load. I used hide-if-js instead, so that in the case of no-JS (broken JS or user choice), they don't have to set the option and save in order to show the rest of the settings.

Committing shortly.

[helen]

In 31095:

Collapse additional avatar settings if avatars are disabled.

props markjaquith, krogsgard, valendesigns.
fixes #30168.

[helen]

In 31560:

Settings API: Allow passing a class to add_settings_field() via the $args array.

While it's possible to target the wrapper element otherwise (currently a tr), this deficiency is made especially noticeable when custom code cannot take advantage of what core is doing, such as with avatars in #30168.

props valendesigns.
fixes #28975. see #30168.