Skip to:
Content

Opened 3 years ago

Closed 3 weeks ago

Last modified 3 weeks ago

#4677 closed enhancement (fixed)

Option to disable Group avatars

Reported by: sooskriszta Owned by: boonebgorges
Milestone: 2.3 Priority: low
Severity: minor Version:
Component: Component - Groups Keywords: has-patch dev-feedback needs-testing
Cc: vivek@…

Description

In Admin, it would be nice to have an option to disable avatars for Groups.

I'm assigning component Groups as opposed to Theme, as it is not just a question of showing the avatar. It is a wholesale question of uploading too - so group creation workflow, group edit options, etc.

Attachments (5)

4677.patch (5.5 KB) - added by boonebgorges 2 years ago.
4677.2.patch (8.3 KB) - added by xgz 8 weeks ago.
Adding options on / off loading avatar in the group and on / off the display on the pages.
4677.3.patch (8.3 KB) - added by boonebgorges 6 weeks ago.
4677.4.patch (10.3 KB) - added by imath 6 weeks ago.
4677.5.patch (13.4 KB) - added by boonebgorges 6 weeks ago.

Download all attachments as: .zip

Change History (36)

comment:1 @boonebgorges3 years ago

  • Milestone changed from Awaiting Review to 1.6.2
  • Priority changed from normal to low
  • Severity changed from normal to minor

This may not warrant a UI toggle, but it's a good idea to make it possible with a constant or a filter.

comment:2 @sooskriszta3 years ago

As a very non-tech-literate user, I always advocate for a bigger admin UI footprint for BuddyPress. The more easily folks can find and use features, the better it is...

comment:3 @Prometheus Fire2 years ago

I agree that a UI toggle seems like a lot of extra work for this. The use cases for this need are limited and not worth having a UI switch, but a filter or define statement in wp-config.php would be great way to turn this off.

Not all BP instances (corporate intranet, for ex.) would need avatars for groups and being able to disable would be a good flexibility.

comment:4 @sooskriszta2 years ago

A wp-config.php statement would be relatively more usable than just a filter.

comment:5 @DJPaul2 years ago

  • Milestone changed from 1.6.2 to 1.7

comment:6 @johnjamesjacoby2 years ago

  • Milestone changed from 1.7 to 1.8

No patch. Punting to 1.8.

comment:7 @boonebgorges2 years ago

  • Keywords has-patch dev-feedback added

4677.patch is a bare-bones pass at implementing a toggle. To toggle, put the following in bp-custom.php:

add_filter( 'bp_enable_group_avatars', '__return_false' );

As you see, it requires some template-level changes. Not ideal, but it's the only way to do it.

Styling probably also needs some tweaks when avatars are absent, especially in the groups directory. Could put in a no-avatar class or something along those lines, or could try reworking the way that the markup and styling works so that the spacing naturally readjusts.

Anyone have thoughts?

@boonebgorges2 years ago

comment:8 @boonebgorges2 years ago

  • Milestone changed from 1.8 to Future Release

Punting this one. If anyone has feedback or suggestions, we can do it for 1.9.

comment:9 @sooskriszta21 months ago

This ought to do it.

comment:10 @sooskriszta13 months ago

  • Cc vivek@… added

comment:11 @sooskriszta11 months ago

  • Cc vivek@… removed

comment:12 @sooskriszta11 months ago

  • Cc vivek@… added

comment:13 follow-ups: @xgz8 weeks ago

  • Priority changed from low to high
  • Severity changed from minor to major

Hello.
I have done what was needed here, how to post updates for approval? What is the procedure?
Thanks.

comment:14 in reply to: ↑ 13 ; follow-ups: @boonebgorges8 weeks ago

Replying to xgz:

Hello.
I have done what was needed here, how to post updates for approval? What is the procedure?
Thanks.

I'm not sure I understand your question - can you clarify?

I don't mind going forward with this change, though the template-level changes mean that for themes overriding the templates in question, the filter won't fully work. (Yet another argument for not adding a UI toggle.) @hnla, do you have any thoughts about styling adjustments that could be made so that the directory and header don't totally break when the avatar is missing?

comment:15 in reply to: ↑ 13 @sooskriszta8 weeks ago

  • Priority changed from high to low
  • Severity changed from major to minor

Replying to xgz:

I have done what was needed here, how to post updates for approval? What is the procedure?

Please do not change Priority and Severity of tickets without prior consent of core team.

You can read the process of submitting patches at https://bitbucket.org/prepzone3/buddypress-patches/wiki/Submit%20a%20patch%20to%20BuddyPress

comment:16 in reply to: ↑ 14 @sooskriszta8 weeks ago

Replying to boonebgorges:

I don't mind going forward with this change, though the template-level changes mean that for themes overriding the templates in question, the filter won't fully work. (Yet another argument for not adding a UI toggle.) @hnla, do you have any thoughts about styling adjustments that could be made so that the directory and header don't totally break when the avatar is missing?

What he has done so far is create a toggle to enable/disable group avatars. The avatars already uploaded, and avatar placeholders do still appear in the theme, which I think defeats the purpose.

comment:17 in reply to: ↑ 14 @hnla8 weeks ago

Replying to boonebgorges:

Replying to xgz:

Hello.
I have done what was needed here, how to post updates for approval? What is the procedure?
Thanks.

I'm not sure I understand your question - can you clarify?

@hnla, do you have any thoughts about styling adjustments that could be made so that the directory and header don't totally break when the avatar is missing?

I can certainly have a look, I'll perhaps wait to see if there is a viable patch though, as not 100% convinced by this as a core feature toggling group avatars, but if we do doubtless we can find a means of ensuring layouts hold up.

@xgz8 weeks ago

Adding options on / off loading avatar in the group and on / off the display on the pages.

comment:18 follow-up: @sooskriszta7 weeks ago

  • Keywords needs-testing added

comment:19 in reply to: ↑ 18 @xgz6 weeks ago

Replying to sooskriszta:
Are you still testing our patch (4677.2.patch)?
Or do you need something from us?

comment:20 follow-up: @boonebgorges6 weeks ago

xgz - Sorry, this is a volunteer project - the maintainers can't always address incoming patches immediately.

As noted earlier, we will need to address the question of templates. Turning off avatar *uploads* means that default avatars will be displayed, with no way of changing them (aside from Gravatar). This does not seem like a good user experience. IMO, if you turn off group avatars, you should remove the avatars from the templates altogether.

comment:21 in reply to: ↑ 20 @sooskriszta6 weeks ago

Replying to boonebgorges:

if you turn off group avatars, you should remove the avatars from the templates altogether.

I think that's what the patch does, though the UI language is sketchy and unclear.

comment:22 @imath6 weeks ago

I'm adding myself to the notifications about this ticket as this could have an impact on the checks i'm doing in #6290 to load/not load the next/new Avatar UI.

@boonebgorges6 weeks ago

comment:23 follow-up: @boonebgorges6 weeks ago

  • Milestone changed from Future Release to 2.3

Replying to sooskriszta:

Replying to boonebgorges:

if you turn off group avatars, you should remove the avatars from the templates altogether.

I think that's what the patch does, though the UI language is sketchy and unclear.

Yeah, I guess you're right - we already do this when avatars are disabled altogether. My apologies for misreading.

4677.3.patch makes a few revisions:

  • Instead of falling back on false when no value has been saved for 'bp-disable-group-avatar-uploads', fall back on the value of the general 'bp-disable-avatar-uploads'. This is for backward compatibility: anyone who currently has avatars disabled across the site will expect them to be disabled after upgrade.
  • I added a very slight change to the stylesheet to make things look decent on group directories without avatars. This involves adding the 'group-no-avatar'/'group-has-avatar' class. This is kinda ugly, but it's the only way I could think of doing it without template-level mods.
  • General cleanup

It'd be nice to get a second set of eyes on this.

comment:24 @imath6 weeks ago

@boonebgorges: i will look at it asap :)

comment:25 in reply to: ↑ 23 @imath6 weeks ago

Replying to boonebgorges:

Great!

  • Instead of falling back on false when no value has been saved for 'bp-disable-group-avatar-uploads', fall back on the value of the general 'bp-disable-avatar-uploads'. This is for backward compatibility: anyone who currently has avatars disabled across the site will expect them to be disabled after upgrade.

Absolutely

  • I added a very slight change to the stylesheet to make things look decent on group directories without avatars. This involves adding the 'group-no-avatar'/'group-has-avatar' class. This is kinda ugly, but it's the only way I could think of doing it without template-level mods.

Group descriptions are best aligned, great.

It'd be nice to get a second set of eyes on this.

Suggestions :

  • I think you should carry on disallowing group avatars if the 'Show avatars' WordPress setting is not available, else it could be a regression of what we've fixed in 2.1 see #5345
  • I think we shouldn't display the activity secondary avatar if the avatar are not allowed for groups. Else, i find it weird to have the mystery man in 'imath posted an update in [mystery man] group'.
  • I think you forgot the Group Profile Photo create step in bp-groups-loader.php.
  • Even if there's no chance the Avatar UI would load, i think i'd feel better if we could use your new function. Else, i'll simply commit a change (using true) on #6290 about bp_avatar_is_front_edit() because unlike the user profile photo, the group profile photo screen is never displayed in this case as no gravatar service is available for groups.

4677.4.patch = 4677.3.patch + the above suggestions.

@imath6 weeks ago

comment:26 @hnla6 weeks ago

Ok I've patched and tested this:

  • Style wise about the best that can be done generically for bp-default styles, adding the classes a good thing.
  • UX/UI doesn't feel right, if you're going to allow to disable avatars then you'll have to remove the group creation step
  • Similarly in terms of markup rendering we have a mixed bag, group header if already had an avatar will still display the parent wrapper elements, group created after disablement won't, Group dir list displays the markup including empty anchor pointing to group if old or new group.

We'll need to wrap all the avatar calls in a new check in the templates if we do this to remove all traces.

We do impose a minor overhead albeit somewhat trivial of having to factor in additional styles possibly for companion sheets.

If we do commit this to core what would be super nice is if we could have a brief write up or editing of existing guides for new UI functionality so that we can maintain up to date Codex references especially for the user guides section, with perhaps a @mention to myself or mercime so that we can record the addition and keep track.

comment:27 @boonebgorges6 weeks ago

imath - this is all great. Thanks so much. I had forgotten/neglected most of these points.

hnla - imath's patch addresses your point about group creation.

I intentionally tried not to touch the templates. But I agree that it's crummy to leave empty markup in there. And now that I'm doing the trick with the new 'group-has-avatar' class, removing the markup itself will be a sort of progressive enhancement, which means it's not such a big deal to mod the templates. 4677.5.patch does what are (I think) all the necessary template checks, and it looks to me like the page flow continues to be fine in these cases.

@boonebgorges6 weeks ago

comment:28 @hnla5 weeks ago

Appears to be fine, no markup and no group creation steps.

Now got to puzzle out quite what to do with the additional space now for lists without avatars and with little descriptive text but hey ho :)

comment:29 @slackbot5 weeks ago

This ticket was mentioned in Slack in #buddypress by imath. View the logs.

comment:30 @boonebgorges3 weeks ago

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

In 9828:

Allow group avatars to be disabled.

Props xgz, boonebgorges, imath, hnla.
Fixes #4677.

comment:31 @imath3 weeks ago

In 9832:

Add missing changes in buddypress-rtl.css

See #4677

Note: See TracTickets for help on using tickets.