Skip to:
Content

BuddyPress.org

#7840 closed defect (bug) (fixed)

bp_is_messages_compose_screen not working with BP Nouveau

Reported by: sbrajesh Owned by: imath
Milestone: 3.1.0 Priority: normal
Severity: normal Version: 3.0.0
Component: Templates Keywords: has-patch commit
Cc: sbrajesh

Description

The following functions don't work when the BP Nouveau template pack is enabled

`
bp_is_messages_compose_screen()
bp_is_messages_sentbox()

`
I am using it on 'bp_template_redirect'

The problem lies in the way BP Nouveau works. It uses hashbang urls and navigation is handled by the Backbone router.

The url structure changes and the conditions fail.

Is there a way to either make these work or remove certain routes using js for the message component. I don't see any way to remove routes from Backbone router.

Attachments (4)

7840.patch (4.8 KB) - added by imath 21 months ago.
7840.pushState.patch (3.2 KB) - added by imath 21 months ago.
7840.pushState.2.patch (4.3 KB) - added by imath 21 months ago.
7840.pushState.3.patch (6.9 KB) - added by imath 20 months ago.

Download all attachments as: .zip

Change History (26)

#1 @DJPaul
21 months ago

  • Milestone changed from Awaiting Review to 3.0.1

#2 @DJPaul
21 months ago

@imath I know you are not spending much time on BuddyPress nowadays, but following the 3.0 release, can you look at this and share any hints you might have how best to fix this?

#3 @imath
21 months ago

  • Keywords reporter-feedback added

@DJPaul Sure, you can count on me to spend all the needed time to help with Nouveau :)

@sbrajesh I agree this one is an interesting challenge, just to understand, could you share a bit more what you're trying to do on template redirect ? Because as it's a single page app in Nouveau, this hook is only fired once : the first time it is loaded.

#4 @sbrajesh
21 months ago

Thank you both of you for looking into it.

My goal is to redirect certain users from compose screen based on their activity and show them a message.

It is possible to hook on the ajax actions and show the specific error message, the client has been insisting on the earlier method.

In past, you could simply hook to bp_template_redirect and do a check. Since the Message component is built like SPA in 'Nouveau', It makes it unusable unless someone opens the message box directly.

Also, In my personal view, It will be a bad idea to have different behaviour for core functions based on template packs. Many plugin which still uses these to check for the compose screen will fail.

#5 @imath
21 months ago

  • Keywords reporter-feedback removed

Thanks a lot for your reply @sbrajesh

Also, In my personal view, It will be a bad idea to have different behaviour for core functions based on template packs. Many plugin which still uses these to check for the compose screen will fail.

imho this will also happen when the BP Rest API will be available ;) Although i think we need to move forward and try to improve the user experience, I totally understand your concern.

The only way i can think of would be to break the Single Page App if a specific filter like bp_nouveau_message_use_multi_page_app is true. I can transport this into the page and do the needed edits into the Backbone router & views to open "legacy" urls instead of Backbone routes. Is this a good plan ?

#6 @DJPaul
21 months ago

I think the point is these core template functions are now broken, which is what Brajesh is saying, and that's the problem.

Question: does the compose screen and sentbox screen have their own permalinks? Could I link directly to them?

#7 @imath
21 months ago

  • Keywords has-patch needs-testing reporter-feedback added

Thanks a lot for your comment & questions @DJPaul : it made me understand a Single Page Application for the Messages component is too ambitious for now :)

Let's all save us some time. 7840.patch is breaking the SPA to load good old school urls and should satisfy everybody.

The site owner still has the possibility to really enjoy the SPA by using this filter :
add_filter( 'bp_nouveau_messages_is_spa', '__return_true' );

@sbrajesh can you confirm it brings back what you are expected ?

@imath
21 months ago

#8 @DJPaul
21 months ago

Question: does the compose screen and sentbox screen have their own permalinks? Could I link directly to them?

I’m asking because if they do, surely we can detect that server-side on page load. And if they don’t, wouldn’t a better enhancement be to add a very simple router and use history.pushState, so that we can detect that server-side?

We may choose to then change the Nouveau Messages URLs to match BP Legacy but still keep it powered by the JS (and not throw away months of work)?

#9 @sbrajesh
21 months ago

Thank you @imath and @DJPaul

Yes, It is working for me but I will agree with @DJPaul on keeping the enhancements on by default if feasible.

Regards
Brajesh

#10 @sbrajesh
21 months ago

To answer the question. Yes, compose screen does have permalinks(partially).

It is used in bp_get_send_private_message_link().

In case of BuddyPress, There does not exist standard template tags for linking to user's groups/friends etc. The normal practice is appending the user domain, component slug and action to get the url.

Case of compose is special that it is used in PM, other's don't have well defined template tags.

#11 @imath
21 months ago

@DJPaul you don’t need me to reply to this! Just use the Messages UI 😊

But YES you can directly reach any box and single message.

eg site.url/username/messages/#compose But as you know PHP don’t get what’s behind #.

#12 @DJPaul
21 months ago

You can get it on page load:

<?php
parse_url('http://site.url/username/messages/#compose')
=> [
     "scheme" => "http",
     "host" => "site.url",
     "path" => "/username/messages/",
     "fragment" => "compose",
   ]

Plugins that would have made changes when a particular screen is loaded, will or should update to support Nouveau, if they want to modify things on the front-end. I agree they can't do back-end changes now. I'm not sure we can support everything.

#13 @imath
21 months ago

Thanks @DJPaul

I think our trouble is more linked to the fact, you won't get the fragment using $_SERVER['REQUEST_URI'] for instance. So I took a deeper look at the Backbone history object. And we could do 7840.pushState.patch. The main trouble is that in this case we need to make sure a trailing slash is not added to the url within bp_redirect_canonical(). This also means, we should remove this trailing slash if the URL contains one. I'm going to look further on this road.

I think, It would do part of the job, but for @sbrajesh trouble, the template redirect hook won't be fired if the click comes from the UI.

#14 @imath
21 months ago

I think 7840.pushState.2.patch is much better to solve the trailing slash thing.

If the need is to add custom message into the UI @sbrajesh This can be done in JavaScript using something like this :

function test_compose_view() {
	wp_add_inline_script( 'bp-nouveau-messages', '
( function( bp ) {

	if ( ! bp.Nouveau || ! bp.Nouveau.Messages ) {
		return;
	}

	Backbone.history.on( \'all\', function( route, router, view ) {
		if ( \'composeMessage\' !== view ) {
			return;
		}

		bp.Nouveau.Messages.displayFeedback( \'Hello Compose View\', \'info\' );
	} );

} )( window.bp || {} )
	' );
}
add_action( 'bp_enqueue_scripts', 'test_compose_view' );

#15 @imath
21 months ago

FYI in 7840.pushState.2.patch bp_is_messages_compose_screen() is now true when the message is ajax sent.
It's true when using the direct url site.url/members/username/messages/compose or clicking on the My Account Admin Bar link.

#16 @imath
21 months ago

  • Owner set to imath
  • Status changed from new to assigned

#17 @sbrajesh
21 months ago

Thank you for the update and fix.

#18 @imath
21 months ago

  • Keywords 2nd-opinion added; needs-testing reporter-feedback removed

Thanks for your feedback, @DJPaul do you think I can commit 7840.pushState.2.patch or do you see some improvements about it ?

#19 @DJPaul
20 months ago

  • Milestone changed from 3.0.1 to 3.1.0

Milestone renamed

#20 @imath
20 months ago

  • Keywords commit added; 2nd-opinion removed

I've found a way to avoid editing core classes (BP_Component & BP_Members_Component) by adding a trailing slash to the routes of the Backbone router.

I'll go with this one!

#21 @imath
20 months ago

In 12115:

BP Nouveau Messages UI: Improve the Backbone router using pushState

In order to be consistent with conditional template tags such as bp_is_messages_compose_screen(), the routes of the Backbone router have been updated to match Messages component canonical URLs

Props sbrajesh, djpaul

See #7840 (Trunk)

#22 @imath
20 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 12116:

BP Nouveau Messages UI: Improve the Backbone router using pushState

In order to be consistent with conditional template tags such as bp_is_messages_compose_screen(), the routes of the Backbone router have been updated to match Messages component canonical URLs

Props sbrajesh, djpaul

Fixes #7840 (Branch 3.0)

Note: See TracTickets for help on using tickets.