WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 5 years ago

Last modified 4 years ago

#30168 closed enhancement (fixed)

Collapse additional avatar settings if avatars are disabled

Reported by: markjaquith Owned by: helen
Milestone: 4.2 Priority: normal
Severity: normal Version:
Component: Comments Keywords: has-patch
Focuses: ui, administration Cc:

Description

https://cldup.com/EZTp-8ph2H-3000x3000.png

Attachments (8)

30168.diff (1.2 KB) - added by markjaquith 5 years ago.
30168.2.diff (1.7 KB) - added by valendesigns 5 years ago.
Loads the JavaScript via the admin_print_footer_scripts action instead of inline.
30168.3.diff (1.7 KB) - added by valendesigns 5 years ago.
30168.4.diff (1.7 KB) - added by valendesigns 5 years ago.
30168.5.diff (1.4 KB) - added by helen 5 years ago.
30168.6.diff (1.5 KB) - added by valendesigns 5 years ago.
30168.patch (1.4 KB) - added by iseulde 5 years ago.
30168.7.diff (3.2 KB) - added by helen 5 years ago.

Download all attachments as: .zip

Change History (26)

@markjaquith
5 years ago

#1 @krogsgard
5 years ago

  • Keywords has-patch added

#2 @SergeyBiryukov
5 years ago

  • Component changed from General to Comments
  • Focuses ui administration added

@valendesigns
5 years ago

Loads the JavaScript via the admin_print_footer_scripts action instead of inline.

#3 @valendesigns
5 years ago

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
5 years ago

#4 @valendesigns
5 years ago

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.

@valendesigns
5 years ago

#5 @johnbillion
5 years ago

  • Milestone changed from Awaiting Review to Future Release
  • Type changed from defect (bug) to enhancement

@helen
5 years ago

#6 follow-up: @helen
5 years ago

  • Milestone changed from Future Release to 4.2

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.

#7 in reply to: ↑ 6 @valendesigns
5 years ago

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!

#8 @nacin
5 years ago

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.

#9 follow-up: @helen
5 years ago

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.

#10 in reply to: ↑ 9 ; follow-up: @valendesigns
5 years ago

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
5 years ago

#11 @valendesigns
5 years ago

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

#12 follow-up: @iseulde
5 years ago

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.

#13 in reply to: ↑ 12 @helen
5 years ago

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
5 years ago

#14 @iseulde
5 years ago

Ok, just adjusted the patch a bit. :)

@helen
5 years ago

#15 in reply to: ↑ 10 @helen
5 years ago

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.

This ticket was mentioned in Slack in #core by helen. View the logs.


5 years ago

#17 @helen
5 years ago

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

In 31095:

Collapse additional avatar settings if avatars are disabled.

props markjaquith, krogsgard, valendesigns.
fixes #30168.

#18 @helen
4 years ago

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.

Note: See TracTickets for help on using tickets.