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

Fix the position of the Table of Contents #97

Merged
merged 4 commits into from
Jun 11, 2024
Merged

Conversation

adamwoodnz
Copy link
Contributor

This PR updates the single page template to use the 3 column layout from the parent theme. This brings alignment with the single pages on Developer Resources, and fixes the incorrect positioning resulting from WordPress/gutenberg#60347. In implementing this layout I've also updated the post title text styles, and moved the search bar to the new position shown in the designs.

Depends on WordPress/wporg-mu-plugins#624 and WordPress/wporg-parent-2021#140
Closes #96
Closes #95

Screenshots

Desktop

Before After
wordpress org_documentation_article_search-engine-optimization_(Desktop) wordpress org_documentation_article_search-engine-optimization_(Desktop) (1)

How to test the changes in this Pull Request:

  1. Easiest to test in sandbox along with the sibling PR for Developer Resources
  2. Check the position of the table of content on page load and when scrolling. Check that the height of the sidebar reduces when coming in contact with the tall footer. Check responsive behaviour.
  3. Check the search bar position across the site

Comment on lines +4 to +13
"settings": {
"custom": {
"alignment": {
"aligned-max-width": "100%"
}
},
"layout": {
"contentSize": "960px"
}
},
Copy link
Contributor Author

@adamwoodnz adamwoodnz Jun 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

<div class="wp-block-group alignfull" style="padding-right:var(--wp--preset--spacing--edge-space);padding-left:var(--wp--preset--spacing--edge-space)">

<!-- wp:group {"align":"left","className":"has-three-columns","layout":{"type":"flex","flexWrap":"wrap","orientation":"vertical"}} -->
<div class="wp-block-group alignleft has-three-columns">
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

3 column layout wrapper added

Copy link
Collaborator

@ryelle ryelle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few minor comments, but overall this fixes the TOC layout, and the increased spacing in articles is nice.

"settings": {
"custom": {
"alignment": {
"aligned-max-width": "100%"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we use this value anywhere? IIRC it's leftover from the long-past (news theme), but i don't recall if we started using it again in devhub

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not that I know of. We keep needing to override it. I'll test removing it in my sandbox.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's used here, but removing this doesn't seem to affect any of the sites I've tested. It is being overridden in all the sites I've worked on (Dev Res, Docs and Learn), so I think it can be safely removed. I think not in this PR though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Created an issue

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think not in this PR though.

Why add it now just to remove it later?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair point 😁 It's just that I've already got 4 PRs to merge for this one fix, and I feel like it needs more testing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've put in in progress on the Learn board, I will continue with it

@adamwoodnz adamwoodnz merged commit 8c3d09f into trunk Jun 11, 2024
1 check passed
@adamwoodnz adamwoodnz deleted the enhance/theme branch June 11, 2024 00:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Component] Theme Templates, patterns, CSS
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Move search bar to top left Update single page template
2 participants