New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix theme compatibility issues for Message comments #2266

Merged
merged 6 commits into from Oct 17, 2018

Conversation

Projects
None yet
2 participants
@alexsanford
Contributor

alexsanford commented Oct 1, 2018

This PR fixes two issues that were found on the Message page on unsupported themes:

  • The Message comments UI was not being displayed for some themes (e.g. Divi). This prevented commenting on Message threads.

  • In some themes (Sydney, Astra) the comments section header included the title of the message. The issue was that the title was blank (Two Comments on "").

Testing

  • Visit the Message page on several themes (/messages/<message_slug>). Ensure that you can see the message comments and add a new comment.

  • If the theme displays the title in the comment section header, ensure that it displays properly.

  • In 5fbfc0b there is a change that potentially touches comments across all of Sensei. It should not have any effect on rendering, but we should test comments on some supported themes and ensure that nothing bad has happened.

@alexsanford alexsanford added this to the 1.12.0 milestone Oct 1, 2018

@alexsanford alexsanford self-assigned this Oct 1, 2018

@alexsanford alexsanford requested review from donnapep and ice9js Oct 1, 2018

@donnapep

This comment has been minimized.

Contributor

donnapep commented Oct 4, 2018

I don't think pagination is quite working for messages.

Using Astra, I see Previous Page on the /messages page. When I click it, it takes me to a different page that I've published on my site.

Also, I have two messages. When I select the first one, I see Previous Message pagination instead of Next Message.

Do you happen to know if pagination existed for messages before? If not, perhaps we shouldn't worry about it here.

@alexsanford alexsanford force-pushed the fix/message-theme-compat-issues branch from dafdbec to 005e572 Oct 16, 2018

Remove theme pagination on all Sensei pages
The individual Sensei handlers will do their own pagination.
@alexsanford

This comment has been minimized.

Contributor

alexsanford commented Oct 16, 2018

@donnapep I dug into this quite a bit.

The pagination/navigation links you saw are coming from the theme. Frankly, I think that Astra has some other problems with these links, because they were acting really weird for me on regular Posts as well.

In any case, I think the solution is to try to turn off the theme's pagination for Sensei pages. I've added that in the latest two commits. Each of the individual handlers adds Sensei's pagination links as needed anyway, so having the theme do it as well is redundant.

Note that this will only work if the theme is using canonical pagination (i.e. using something like the_post_navigation() which eventually calls get_previous_post_link() and/or get_next_post_link()). If the theme does something custom then there's not much we can do.

Anyway, please re-test this and see if it works for you.

@donnapep

This comment has been minimized.

Contributor

donnapep commented Oct 17, 2018

Note that this will only work if the theme is using canonical pagination

How will that manifest on the front-end if the theme is not using canonical pagination? It could just mean some messed up pagination as we saw with Astra?

@alexsanford

This comment has been minimized.

Contributor

alexsanford commented Oct 17, 2018

@donnapep

It could just mean some messed up pagination as we saw with Astra?

Yes, exactly. It would just mean that my code to turn off theme-based pagination doesn't work, and the theme will show its pagination. Which may or may not be as messed up as Astra 🙂

@donnapep

Let's put out Beta 2. 🙂

@alexsanford alexsanford merged commit 9dbafa5 into master Oct 17, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@alexsanford alexsanford deleted the fix/message-theme-compat-issues branch Oct 17, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment