[buddypress-trac] [BuddyPress Trac] #6331: Star Private Messages

[buddypress-trac]

#6331: Star Private Messages
------------------------------------+-----------------------
 Reporter:  r-a-y                   |       Owner:  r-a-y
     Type:  enhancement             |      Status:  assigned
 Priority:  normal                  |   Milestone:  2.3
Component:  Component - Messaging   |     Version:
 Severity:  normal                  |  Resolution:
 Keywords:  dev-feedback has-patch  |
------------------------------------+-----------------------

Comment (by r-a-y):

 Thanks for everyone's feedback, particularly imath's!

 > Is there a specific reason not to hook bp_enqueue_scripts to fire
 bp_messages_starred_enqueue_styles()

 Good spot!  Will make this change.

 > Maybe this `add_action( 'wp_ajax_messages_starred',
 'bp_messages_starred_ajax_handler' );` should live in `bp-templates/bp-
 legacy/buddypress/buddypress-functions.php` :

 I was thinking about that as well.  I'm happy to go with what everyone
 else thinks here.

 > When you require the bp-messages-starred.php file is there a specific
 reason not to do that :
 > if ( bp_messages_is_starred_enabled() ) {
 >       $includes[] = 'starred';
 > }

 This is what I wanted to do originally, but we need to run the
 `parent::includes()` function first since the
 `bp_messages_is_starred_enabled()` function itself lives in `bp-messages-
 functions.php`.

 Unless we move `bp_messages_is_starred_enabled()` into `bp-messages-
 loader.php`.  Not sure if we want to do that.

 > There's a problem with your filter to disable the functionality. I think
 you should either use `add_filter( 'bp_messages_enable_starred',
 '__return_false' );`

 Happy to change the filter to `'bp_messages_enable_starred'`, but if we
 change the filter name, doesn't the function name have to be the same name
 since we usually have the filter and function name be the same?

 > `bp_messages_is_starred_disabled()`

 I think we do not want to have negative-sounding functions.  I think we
 had a similar discussion regarding the "disable activity stream commenting
 on blog and forum posts" option in the BP admin area and how we switched
 that over to the opposite.

 > Your gif was giving me the impression the star feature was another
 messages action just like read, unread, or delete, viewing it in
 twentyfifteen i see it's a very particular action requiring it's own
 column.

 Another good call here, imath.

 I kind of wanted to move the "Star" column near the checkbox, but since
 the "Unread / Read / Delete" links are at the end of the table, I left the
 Star column by the existing actions.

 Personally, I would love to change the "Unread / Read" and "Delete" links
 into icons, so the "Star" icon could go alongside them.  Or maybe even
 remove the "Actions" column altogether and just leave those actions as a
 bulk action.  This is what GMail does.

 Would love to hear what everyone thinks about this.

 ----

 > my personal preference would be to find
 `bp_messages_starred_enqueue_styles()` in `bp-messages-cssjs.php` just
 like it's the case for `bp_activity_mentions_script()` for instance, to
 find the code in the function `bp_messages_starred_message_css_class()`
 inside `bp_get_the_thread_message_css_class()` the screen function inside
 `bp-messages-screens.php` etc.. Because as a plugin developer wishing to
 extend BuddyPress i'm used to this organization :)

 I'm not opposed to moving the CSS class and enqueue styles stuff into the
 core BP messages component.  I'll move it in just for you, imath :)

 > When viewing my starred tab (Inbox | Starred | Sent | Compose) i would
 expect to see a list of the item i've starred, not a list of the thread
 containing one or more item i starred.

 > imho if the target is the messages item, the column in inbox/sent should
 only be informative, and the bulk action should live in the starred tab
 which should only display the starred items (so not the parent thread),

 I copied this functionality directly from GMail, but as I was implementing
 this I can see your point.

 One change I made from GMail's functionality is if you star a thread, it
 stars the first message in the thread.  In GMail, if you star a thread, it
 stars the last message in the thread.  I found this awkward, so I made it
 star the first message.

 GMail also has an option to toggle conversation view.  When conversation
 view is disabled, all loops change to display individual messages.
 However, when conversation view is enabled, all loops change to threads.
 Since BP's mode is effectively GMail's enabled conversation view, this is
 what I followed.

 It might be cool to have a "Conversation view disabled" mode, then we can
 do what you propose.  However, this would probably require changing up the
 main `/members/single/messages.php` template and also changes to how we
 fetch messages.


 > i understand why it's not in the bulk select box, but imho, as a user i
 think it's an action just like the others.

 I'll look into adding the star actions into the bulk actions dropdown
 menu.  This was an oversight on my part!

 > (maybe a title attribute into the star link to inform "Delete all
 starred items" ?)

 Will implement this.

 ----

 > Or if we are to do this, then maybe a folder should be used to organize
 them a bit like DJPaul did with the components classes eg: `/classes
 /class-bp-messages-message.php`

 .

 >  disabling the feature will result in the file's not being loaded; this
 seems like a big improvement from the point of view of performance and
 modularity.

 This is what I wanted feedback on.  Thanks for chiming in!

 Just to be clear, I do not want to reorganize all existing functionality
 to this new method.  Only new features going forward and when it makes
 sense to do so.  Since the BP codebase continues to grow with awesome new
 features, I just want to make sure we do not follow the route of WP and
 including everything on each page load.  If a dev wants to turn something
 off, then they should be able to do so without the extra code being
 loaded.

 That being said, should the file live in a `/modules/` subdirectory or
 something similarly named?  I'd love to get the opinion of all core devs
 on this because this is a new approach to development.

 I was thinking about leaving the code I wrote as a plugin into classes so
 it could live in `/classes/`, but this goes against how all BP code is
 written.  So that's a no-go.


 > Regarding naming. bp_messages_is_starred_enabled() is a bit awkward when
 spoken.

 Thanks for this.  I was using "starred" because of the slug name, but I
 can switch to "stars" instead of "starred" for everything.

 Thanks again for the feedback everyone.

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