Skip to content
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

Related Posts: Adjust margins due to WordPress 5.3 #14020

Closed
nielslange opened this issue Nov 13, 2019 · 15 comments · Fixed by #14066
Closed

Related Posts: Adjust margins due to WordPress 5.3 #14020

nielslange opened this issue Nov 13, 2019 · 15 comments · Fixed by #14066
Assignees
Labels
[Feature] Related Posts [Feature] Theme Tools [Pri] High [Type] Bug When a feature is broken and / or not performing as intended
Milestone

Comments

@nielslange
Copy link
Member

Steps to reproduce the issue

  1. Go to /wp-admin/admin.php?page=jetpack#/traffic and ensure that Related posts are activated
  2. Go to /wp-admin/post-new.php and create at least four test posts that belong to the same category
  3. Go to / and open one of the posts
  4. See error

What I expected

The related posts are centred

What happened instead

The related posts are left aligned

Screenshots

jetpack-related-posts-before

@nielslange nielslange added [Type] Bug When a feature is broken and / or not performing as intended [Feature] Related Posts labels Nov 13, 2019
@nielslange nielslange self-assigned this Nov 13, 2019
@jeherve
Copy link
Member

jeherve commented Nov 13, 2019

Could you try to update to Jetpack 7.9? The latest version of Jetpack includes a compatibility stylesheet that ensures that related posts (amongst other things) are displayed nicely in the Twenty Twenty theme.

If that doesn't seem to work for you, I'd recommend that you check 2 things:

  • Your copy of Twenty Twenty should be the official one, and not one you may have downloaded from GitHub (if downloaded from GitHub, the theme may be named twentytwenty-master instead of twentytwenty)
  • If you use a caching plugin, make sure to flush your cache to ensure that all CSS is up to date.

Related: #13516

@jeherve jeherve closed this as completed Nov 13, 2019
@nielslange
Copy link
Member Author

nielslange commented Nov 13, 2019

@jeherve I'm already using Jetpack 7.9 as well as the Twenty Twenty theme from the release instead of the once from the GitHub repo. The issue was visible on my site https://nielslange.blog/, before I fixed it. While I used a modified version of the CSS, to make let the related posts appear wider, I first created the following CSS code to fix this issue:

.entry-content #jp-relatedposts {
  margin: 1em auto;
}

I'm aware of the compatibility stylesheet and I cannot see the necessary fix in it. That's why I created this issue as well as a fix for it. Technically, my PR to fix this issue is ready to be pushed. But from what I understand, you're unable to reproduce this issue on your end. Is this correct?

UPDATE: I just created a fresh installation on https://2020.nielslange.de/2019/11/13/wordpress-5-3-kirk/ with WordPress 5.3 and Jetpack 7.9. Same issue. 🧐(The related posts are currently only visible in the customizer though. It might take a bit before Jetpack starts showing them on the frontend as well.)

@jeherve
Copy link
Member

jeherve commented Nov 13, 2019

Interesting. I don't see the issue on my own site, but I can reproduce on a fresh site as well, and I see on @kraftbj's site too. I wonder if this may be caused by the different alignment changes that landed in Core on release day yesterday:
https://core.trac.wordpress.org/ticket/48571

In any case, that's definitely an issue right now. Reopening.

@nielslange
Copy link
Member Author

Interesting. I don't see the issue on my own site, but I can reproduce on a fresh site as well, and I see on @kraftbj's site too. I wonder if this may be caused by the different alignment changes that landed in Core on release day yesterday:
https://core.trac.wordpress.org/ticket/48571

In any case, that's definitely an issue right now. Reopening.

Based on your reply, I just pushed my PR to #14023. Just let me know if something needs to be adjusted, @jeherve.

@andersnoren
Copy link

@jeherve @nielslange There were changes made to the block structure of Twenty Twenty a day before release, which changed it from an entry-content wrapper with a max-width to a max-width on each element in the .entry-content. The max-width in the theme is applied correctly to #jp-relatedposts, but the margin property is overwritten.

Changing #jp-relatedposts from margin: 1em 0; to margin: 1em auto; should fix it. Sorry about this causing issues.

@nielslange
Copy link
Member Author

nielslange commented Nov 14, 2019

@andersnoren Fixed it already! We just need @jeherve to 🚢 the fix! 😁

@macmanx2
Copy link
Contributor

I can also confirm that this fix works. :)

@scottsweb
Copy link
Contributor

Noting that this change to the theme may also be impacting the GIF block:

Screenshot 2019-11-18 at 13 07 02

@nielslange
Copy link
Member Author

@scottsweb You mean this fix would also fix the GIF block issue, right?

@scottsweb
Copy link
Contributor

I am just testing your PR @nielslange, I think we will need separate CSS on the GIF block too. I think we may need to test all our blocks. Just having a look at some potential fixes.

@nielslange
Copy link
Member Author

Thanks for clarifying this, @scottsweb!

@scottsweb
Copy link
Contributor

Found another block issue, but it is different enough to warrant a different issue/PR. #14063

Still working through the blocks we have and testing them.

@nielslange
Copy link
Member Author

nielslange commented Nov 18, 2019

Closing a proposed PR (#14020) and opening and merging an almost identical PR (#14066) which is based on the proposed PR send false signals to potential contributors. It creates the impression that the proposed PR is not considered valuable enough and might lead to the fact this repo loses contributors.

Personally, seeing this happening here makes me think twice of contributing to Jetpack again. That's not how Open Source should be like, IMO.

@jeherve
Copy link
Member

jeherve commented Nov 19, 2019

@nielslange Thanks for the feedback! We usually don't take that approach. Unfortunately, it appears that you had created your PR from a branch on your own fork, and did not check this box when creating your PR, thus making it a bit more difficult to collaborate with you on a fix.

image

Under regular circumstances, that wouldn't have been such a big issue; we could have waited and coordinated to help you push additional commits to your own branch. Unfortunately, we're a bit under pressure as we want to include this fix in a release that will be pushed later today, so we had to move a bit quicker than usual on this.

As an a11n, do not hesitate to create your PRs from a branch pushed to the main Jetpack repo, that's totally fine! :) Also feel free to join us in #jetpack-plugin if you want to chat about this a little bit more.

@scottsweb
Copy link
Contributor

Apologies if it came across that way @nielslange, as I mentioned on your PR #14023 I was unable to push to your fork which meant I had to spin up something new... which I don't like to do for this reason 😞

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Related Posts [Feature] Theme Tools [Pri] High [Type] Bug When a feature is broken and / or not performing as intended
Projects
None yet
5 participants