Skip to:
Content

BuddyPress.org

Opened 6 years ago

Closed 6 years ago

#7840 closed defect (bug) (fixed)

bp_is_messages_compose_screen not working with BP Nouveau

Reported by: sbrajesh's profile sbrajesh Owned by: imath's profile 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 6 years ago.
7840.pushState.patch (3.2 KB) - added by imath 6 years ago.
7840.pushState.2.patch (4.3 KB) - added by imath 6 years ago.
7840.pushState.3.patch (6.9 KB) - added by imath 6 years ago.

Download all attachments as: .zip

Change History (26)

#1 @DJPaul
6 years ago

  • Milestone changed from Awaiting Review to 3.0.1

#2 @DJPaul
6 years 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
6 years 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
6 years 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
6 years 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
6 years 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
6 years 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
6 years ago

#8 @DJPaul
6 years 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
6 years 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
6 years 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
6 years 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
6 years 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
6 years 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
6 years 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
6 years 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
6 years ago

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

#17 @sbrajesh
6 years ago

Thank you for the update and fix.

#18 @imath
6 years 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
6 years ago

  • Milestone changed from 3.0.1 to 3.1.0

Milestone renamed

#20 @imath
6 years 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
6 years 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
6 years 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.