[buddypress-trac] [BuddyPress Trac] #6290: Avatars, an extensible UI

[buddypress-trac]

#6290: Avatars, an extensible UI
------------------------------------+-----------------------
 Reporter:  imath                   |       Owner:  imath
     Type:  idea                    |      Status:  assigned
 Priority:  normal                  |   Milestone:  2.3
Component:  API - Avatars           |     Version:
 Severity:  normal                  |  Resolution:
 Keywords:  has-patch dev-feedback  |
------------------------------------+-----------------------

Comment (by DJPaul):

 My Backbone isn't good enough for me to feel confident giving any code
 review on any of that, so I'm skipping it.

 When this started, I was envisaging a system more like the group extension
 or widget system, where you can easily add new screens. It looks like all
 this Backbone templating has been added mainly because that's how Plupload
 has been implemented into WordPress Media Library?

 If Jetpack wanted to add a BP Profile Pictures module that would let
 people choose/upload new Gravatars to WordPress.com/Gravatar.com, can they
 do that with this system? If so, would the Gravatar upload option appear
 alongside the traditional upload form, and the new web camera form?

 ---


 > I love your system for overridable Backbone templates - much more
 elegant than WP's.

 @boonebgorges - what does WP do? I don't know anything about WP's
 Backbone.

 > empty()

 This is not specific to this ticket, but the recent increase in mis-using
 empty() is annoying. `! empty()` is the same as doing an `isset` and `x !=
 ''` check.  In this patch, for example, we have:

 {{{
 function bp_attachments_enqueue_scripts( $class = '' ) {
         // Enqueue me just once per page, please.
         if ( did_action( 'bp_attachments_enqueue_scripts' ) ) {
                 return;
         }

         if ( empty( $class ) || ! class_exists( $class ) ) {
                 return new WP_Error( 'missing_parameter' );
         }
 }}}

 An empty `$class` string will evaluate to `false`, so let's just use `!
 $class`. :)

 There's also a few type insensitive comparision functions in this patch
 (`==` instead of `===`), let's not get sloppy.
 And I will give you a miilion euros if you please do not add any more
 `@uses` phpDoc into BuddyPress, the most useless of all phpDoc tags. I
 will open a ticket to petition for removal of them, eventually. :)

 > bp_attachments_get_plupload_l10n()

 Are the strings copied from WordPress core?

 > i18n

 In `bp_attachments_enqueue_scripts`, in a user-facing string, we refer to
 "avatars". We decided to use "Profile Photo" (or something) instead of
 avatar a couple of versions ago (like you've done in other parts of the
 patch). :)

 > bp_core_avatar_is_front_edit_avatar

 There are probably other funtion names in this pattern, but I don't like
 this one. Are you trying to "namespace" everything inside core and an
 avatar categories? Why not just "bp_avatar_"?

 > bp_core_avatar_template_check

 I don't think we should do this. Throwing PHP Notices for user feedback is
 lousy. I recall someone saying this somewhere already, but I can't find it
 on the Trac ticket -- maybe it was on Slack? Not sure.

 > bp_is_my_admin_profile

 Needs unit test. I think the "are we in wp-admin/users.php" check should
 be simplified by using `is_admin` etc and `get_current_screen`.

--
Ticket URL: <https://buddypress.trac.wordpress.org/ticket/6290#comment:22>
BuddyPress Trac <http://buddypress.org/>
BuddyPress Trac