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

Data Liberation: Add interior pages #375

Open
wants to merge 39 commits into
base: trunk
Choose a base branch
from

Conversation

StevenDufresne
Copy link
Contributor

@StevenDufresne StevenDufresne commented Dec 20, 2023

This PR adds an interface for the guide location in the https://github.com/WordPress/data-liberation repository.

Screenshots

Header Header
Home
Archive localhost_8888_data-liberation_and_ (1)
Single localhost_8888_data-liberation_and_rss-4_

How to test the changes in this Pull Request:

@StevenDufresne StevenDufresne changed the title Add the guide list pages. Data Liberation: Add interior pages Dec 20, 2023
@StevenDufresne StevenDufresne force-pushed the add/data-liberation-child-pages branch from 46538c9 to 0e731ca Compare December 21, 2023 04:53
@StevenDufresne StevenDufresne force-pushed the add/data-liberation-child-pages branch from e4969f3 to c94e868 Compare January 15, 2024 05:21
@StevenDufresne StevenDufresne force-pushed the add/data-liberation-child-pages branch from 3911644 to 6238975 Compare January 22, 2024 04:50
@StevenDufresne StevenDufresne marked this pull request as ready for review January 22, 2024 07:03
@StevenDufresne StevenDufresne requested review from adamwoodnz, ryelle, renintw and a team January 22, 2024 07:07
Copy link
Contributor

@renintw renintw left a comment

Choose a reason for hiding this comment

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

Looks good to me, mostly works as described. 👍
Left some comments.
And we can consider aligning the post title with the top of the TOC.

image
image


<!-- wp:pattern {"slug":"wporg-main-2022/nav-data-liberation-single"} /-->


Copy link
Contributor

Choose a reason for hiding this comment

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

extra line.

),
array(
'url' => false,
'title' => get_the_title(),
Copy link
Contributor

Choose a reason for hiding this comment

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

This would somehow get me the title plus an edit button, is this also the case on your side?

image

Copy link
Contributor

Choose a reason for hiding this comment

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

I also see this, looks like it comes from filter_the_title_edit_link in the wporg-markdown plugin.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is everyone okay with removing that function from the wporg-markdown plugin?

Context: #375 (comment)

Copy link
Contributor

@renintw renintw Jan 24, 2024

Choose a reason for hiding this comment

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

If it's not going to be used anywhere, I also think we can remove it.

@@ -30,56 +30,56 @@

<!-- wp:group {"style":{"spacing":{"blockGap":"var:preset|spacing|10"}},"className":"is-style-cards-grid","layout":{"type":"grid","minimumColumnWidth":"32%"}} -->
<div class="wp-block-group is-style-cards-grid"><!-- wp:wporg/link-wrapper -->
<a class="wp-block-wporg-link-wrapper" href="https://github.com/WordPress/move-to-wp/blob/trunk/guides/squarespace-to-wordpress.md"><!-- wp:heading {"level":3,"style":{"spacing":{"margin":{"top":"0","bottom":"0"}},"typography":{"fontStyle":"normal","fontWeight":"700"}},"fontSize":"small","fontFamily":"inter"} -->
<a class="wp-block-wporg-link-wrapper" href="<?php echo esc_url( site_url( 'data-liberation/and/squarespace' ) ); ?>"><!-- wp:heading {"level":3,"style":{"spacing":{"margin":{"top":"0","bottom":"0"}},"typography":{"fontStyle":"normal","fontWeight":"700"}},"fontSize":"small","fontFamily":"inter"} -->
Copy link
Contributor

Choose a reason for hiding this comment

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

This page is synced from the production site, so the PHP here would be overwritten when the script is run. Should it be removed from the syncing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The hardcoded URL is a temporary solution.

An ideal solution is to implement tags/categories to control that list.

There are 2 reasons why I resorted to hardcoded items:

  • We don't have categories set up
  • The <LinkWrapper> component that was introduced in 685b3c8, doesn't play nicely with the <PostTemplate> component.

So no, they should not be removed.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't mean to remove the items themselves, but that the PHP here will be overwritten when yarn build:patterns is run again. So it should either be changed to href="/data-liberation/and/squarespace/" in the page content & re-synced, or this page removed from page-manifest.json and disconnect it from the syncing process.


<!-- wp:group {"tagName":"main","style":{"spacing":{"blockGap":"0px"}},"className":"entry-content","layout":{"inherit":true,"type":"constrained"}} -->
<main class="wp-block-group entry-content">
<!-- wp:pattern {"slug":"wporg-main-2022/data-liberation-guides"} /-->
Copy link
Contributor

Choose a reason for hiding this comment

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

Are there plans to support translating the data lib pages? If not, we probably don't need to use the pattern process here, since these pages wouldn't be rolled out to rosetta sites. (IIRC we don't have a process for translating handbooks yet, which is why i assume no)

Not a strong suggestion, but it would be one less abstraction, to put the pattern content (query loop, etc) here.

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 default to yes. We should support it but understand that rosetta experiences vary right now and these pages may never be viewed in other locales... but still feel it should be localized.

Copy link
Contributor

Choose a reason for hiding this comment

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

okay, if we assume that it will be available eventually then 👍🏻 — my aim was to not make more work for the translators.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good thought and something I haven't considered when developing. Thanks.

add_filter( 'wporg_handbook_toc_should_add_toc', '__return_false' );

// Remove the edit link from handbook titles.
add_filter( 'wporg_markdown_should_filter_title', '__return_false' );
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder why this filter seems to not exist anymore… I would have thought this would prevent the issue @renintw and I are seeing here. I can't find the filter in the wporg-markdown plugin.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, dang. That was a filter I implemented in my local environment in the markdown plugin. Essentially returning early here.

I discussed this with @dd32 and since we don't use the title anywhere anymore, we agreed we could remove the function altogether and I forgot to update.

@adamwoodnz
Copy link
Contributor

adamwoodnz commented Jan 24, 2024

aligning the post title with the top of the TOC

Can be achieved by setting the top margin of the sidebar container via theme.json, see https://github.com/WordPress/wporg-developer/blob/trunk/source/wp-content/themes/wporg-developer-2023/theme.json#L162

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants