[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 imath):

 Hi r-a-y

 Very nice work and very interesting feature! So great to give some new
 functionalities to the messages component.

 > I'd preferably like all new functionality to follow a similar approach
 for upcoming BP features
 imho, i'm not sure. I see the interest of the approach, maybe i'm not used
 to it, but i'd say if it's a core feature, then it should use the
 available files eg: put the templates thing in bp-messages-templates. Else
 it's something hibryd between a plugin and a core feature, and i can
 imagine the number of specific files that would be added for each
 functionality.
 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`, and to be consistent, we would then need
 to do this for all existing functionalities eg: activity mentions.... So
 it looks like a huge reorganization to me :)

 Some feedbacks :
 - Is there a specific reason not to hook {{{bp_enqueue_scripts}}} to fire
 `bp_messages_starred_enqueue_styles()`, i see you are using
 {{{wp_enqueue_scripts}}} ?
 - i can see an interest to allow filtering the registered icons
 stylesheet. If people want to use the icon stylesheet in use for their
 active theme. So it may be interesting to do an `apply_filters(
 'bp_messages_icons_styles', 'dashicons' )`
 - Maybe this `add_action( 'wp_ajax_messages_starred',
 'bp_messages_starred_ajax_handler' );` should live in `bp-templates/bp-
 legacy/buddypress/buddypress-functions.php` :
 {{{
 $actions = array(

         // Directory filters

         // Friends

         // Activity

         // Groups

         // Messages
         /* other ajax actions about the messages component */
         'messages_toggle_star'              =>
 'bp_legacy_theme_ajax_messages_star_toggle',
 );
 }}}
 - 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';
 }

 parent::includes( $includes );
 }}}
 - 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' );` or edit the function
 `bp_messages_is_starred_enabled()` so that the apply_filter is
 'bp_messages_is_starred_enabled'. Imho this should be the opposite as it's
 a core feature, it's on, by default, so people might want to disable it,
 maybe `bp_messages_is_starred_disabled()` ?
 - 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. I personaly think it's complicating the process. Why not use the
 actions column like this :
 {{{
 <td class="thread-options">
         <?php
         /* since BuddyPress 2.3.0 */
         bp_message_thread_star_item() ;?>

         <?php if ( bp_message_thread_has_unread() ) : ?>
                 <a class="read" href="<?php
 bp_the_message_thread_mark_read_url();?>"><?php _e( 'Read', 'buddypress'
 ); ?></a>
         <?php else : ?>
                 <a class="unread" href="<?php
 bp_the_message_thread_mark_unread_url();?>"><?php _e( 'Unread',
 'buddypress' ); ?></a>
                 <?php endif; ?>
                          |
                 <a class="delete" href="<?php
 bp_message_thread_delete_link(); ?>"><?php _e( 'Delete', 'buddypress' );
 ?></a>
 </td>
 }}}

 You would make sure it's the first action, do the checks whether to
 display the star or not based on the avaibility of the feature, and it
 would be a lot more easy for theme designers to edit their templates by
 simply adding the new template tag.
 - As explained earlier, 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 :)
 - It may be interesting in the future to add new actions/message status
 like "read it later, important.." Maybe the star feature could be use to
 build a more generic message status API ?

 === As a user ===

 -  i would expect to be able to bulk unstar/star. As you made this feature
 a particular action, 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.
 - 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. If it's too complex, i guess
 it might be better to only allow the thread to be starred at the begining
 ? Because in the case of an item, i need to open the thread and look for
 my starred item. The star item can be hard to find, so maybe it would be
 interestring to stick them to the top :)

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