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

TT1 Blocks: Clean up for Theme Repo #149

Merged
merged 22 commits into from
Jan 7, 2021
Merged

Conversation

kjellr
Copy link
Collaborator

@kjellr kjellr commented Dec 17, 2020

Takes care of a few of the checklist items from #140 (comment):

  • Update screenshot
  • Update the remaining @subpackage
  • Add file header to functions.php

I also added a first draft of a readme.txt. Please suggest any edits!


For the screenshot, this was a (very) quick version. @melchoyce I'm curious if you have any ideas. I'm not sure how totally different we want the screenshot, considering it's meant to be the exact same theme visually. 😄

Original TT1 Blocks
screenshot screenshot

@kjellr kjellr added the enhancement New feature or request label Dec 17, 2020
@kjellr kjellr self-assigned this Dec 17, 2020

== Changelog ==

= 1.0 =
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure what to do about the version number here. Should we start below 1.0?

Copy link
Collaborator

Choose a reason for hiding this comment

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

0.1

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Works for me. Updated in 582d892.

@kjellr kjellr changed the title Tt1/cleanup for release TT1 Blocks: Clean up for Theme Repo Dec 17, 2020
@melchoyce
Copy link
Collaborator

How about including the tagline and showing a little more of the image?

screenshot

@kjellr
Copy link
Collaborator Author

kjellr commented Dec 17, 2020

Works for me, thanks! PR is updated.

@kjellr
Copy link
Collaborator Author

kjellr commented Dec 17, 2020

I just pushed up archive and search results templates:

Archive Search Results
Screen Shot 2020-12-17 at 2 44 06 PM Screen Shot 2020-12-17 at 2 26 17 PM

Note that the search and archive page titles are static because dynamic blocks for those titles don't exist yet. That will eventually be handled by the "Query Title" block as noted in WordPress/gutenberg#22724.

I'm not 100% sure about the query block markup there, but it seems to work in my testing. Also note that I think Gutenberg 9.6 is required to get these queries working.

@kjellr
Copy link
Collaborator Author

kjellr commented Dec 17, 2020

If those templates look good, I think this will now close #140.

@jffng
Copy link
Collaborator

jffng commented Dec 17, 2020

I tested the Archive and Search templates against GB master, and looks good to me.

@scruffian
Copy link
Collaborator

Is there a reason that we need these extra templates? If we need them, maybe we could create another template part which contains the code that is shared across all these templates?

@kjellr
Copy link
Collaborator Author

kjellr commented Dec 18, 2020

Until the page headers are dynamic, I'm not sure it makes sense to include these additional templates at all — they're basically just the same as the index template (I can easily modify the query there instead of on these two pages, and it will work in all contexts).

@carolinan, you'd included these two templates in your list here. What do you think?

- Tidy up the query itself.
- Fix wide/full styles for inner content in the query block.
@@ -46,7 +46,8 @@ body {
}

.wp-site-blocks > *:not(.wp-block-post-content),
.wp-site-blocks .wp-block-post-content > * {
.wp-site-blocks .wp-block-post-content > *,
.wp-block-query-loop li > *:not(.entry-content):not(.alignwide):not(.alignfull) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

did this creep in by mistake?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, I just added that. Previously, all content of the query block was showing up as standard width, even when it was wide/full. 😩 I noticed this when I moved the default query into its own template part, and had to do this to fix it.

Here's an example, where the Media & Text block is supposed to be wide width:

Before f4a0131 After I placed the query block in the full-width template part. Final result, after I added this css.
Screen Shot 2020-12-18 at 9 12 03 AM Screen Shot 2020-12-18 at 9 11 24 AM Screen Shot 2020-12-18 at 9 11 09 AM

(I really can't wait until this alignments stuff "just works" 😞).

@kjellr
Copy link
Collaborator Author

kjellr commented Dec 18, 2020

BTW, I fixed the global query in f4a0131 by using the usual query block markup, and ensuring that it was set to "inherit":true (Thanks @ntsekouras for the help). Now the index, archive, and search templates all use the same exact query.

@kjellr
Copy link
Collaborator Author

kjellr commented Jan 4, 2021

In functions.php line 216 this text domain is incorrect:

Thanks, should be fixed now.

If there is any code borrowed from for example Bosco, Gutenberg starter theme, Q or emptytheme then those themes and theme developers should be credited too.

I don't think there is anymore. The alignments came from here originally, but they've been rewritten by @scruffian recently. The rest of the base of the theme was just built from scratch or copy/pasted from the original Twenty Twenty-One.

I don't think these two are needed since they are also set in the json file?

Good call. 👍

This should be ready for another review!

@carolinan
Copy link
Collaborator

There are functions in the removed header.php and footer.php files that are needed to pass theme check for the upload to the theme directory.

@carolinan
Copy link
Collaborator

@Otto42 since this theme will be added to the theme directory from the wordpressorg account does it still need to pass theme check?

@carolinan
Copy link
Collaborator

carolinan commented Jan 5, 2021

The post comments block uses comments_template so the theme still needs a comments.php file:

Deprecated: Theme without comments.php is deprecated since version 3.0.0 with no alternative available. Please include a comments.php template in your theme. in /var/www/html/wp-includes/functions.php on line 5059

Update:
I made a separate PR for thi,s #157 because I'm not really happy with adding that much extra CSS. The purpose is only to make sure that the comments are working.

@kjellr
Copy link
Collaborator Author

kjellr commented Jan 5, 2021

There are functions in the removed header.php and footer.php files that are needed to pass theme check for the upload to the theme directory.

It's possible I added those intentionally and forgot why over the holidays. 😄

In any case, even with those files, the theme fails ThemeCheck with a ton of errors:

Screen Shot 2021-01-05 at 8 32 59 AM

So if it does need to pass ThemeCheck, we need to add many more (unused) selectors and functions. If possible, it would be a much better solution to fix ThemeCheck so it doesn't require all this stuff for block based themes.

@carolinan
Copy link
Collaborator

carolinan commented Jan 5, 2021

Without finalized requirements we can't update Theme Check -we won't know what it needs to check for. I estimate that to take at least 3 months.

(The required CSS classes listed there has been removed but that plugin version has not been released yet.)

@scruffian
Copy link
Collaborator

There are other Block Themes in the directory that didn't pass the check...

@carolinan
Copy link
Collaborator

Ah, no that is not how the theme upload works, you have have to pass Theme Check once.
Once the theme data is saved (it is a custom post type) and the files are uploaded, the moderators need to log in and enable a "Special Case Theme" setting in the admin area. This means that theme is not checked by Theme Check on the next upload.
After that, the files can be removed from the theme.

That is why I wanted to ask Otto because I believe he mentioned that there is an exception for default themes. This is not a default or bundled theme but it will be uploaded from the same account.

@kjellr
Copy link
Collaborator Author

kjellr commented Jan 5, 2021

Ok cool. I think the plan was to upload a generic theme with the TT1 Blocks name to start with (a version that passes ThemeCheck), and then once a moderator enables that special case, we can upload the actual version. That should work, right?

@carolinan
Copy link
Collaborator

Yes.

@kjellr
Copy link
Collaborator Author

kjellr commented Jan 5, 2021

Cool. If that's the case, I think we can leave out those header.php and footer.php files. If folks are ok with the rest of what's in this PR, we should be able to get the submission process moving soon.

@carolinan
Copy link
Collaborator

I was hoping to be able to merge this now but when I view an archive or search result I see the following:

Warning: A non-numeric value encountered in /var/www/html/wp-content/plugins/gutenberg/build/block-library/blocks/query-loop.php on line 45

Line 45 is the per page setting:
https://github.com/WordPress/gutenberg/blob/master/packages/block-library/src/query-loop/index.php#L45

In index.html, the per page setting is "query":{"perPage":"",

I am not sure if it has been discussed already but the default value for $posts_per_page is 10 and I think this should reflect that.

Update the perPage setting in the query
@carolinan carolinan requested a review from scruffian January 6, 2021 08:21
@carolinan
Copy link
Collaborator

carolinan commented Jan 6, 2021

Has the lack of query pagination been discussed? Is not including pagination something you feel as strongly about as the comments?

The pagination does not have a CSS class on the front so we can not add any styles for it until the block is updated. But navigating between the archive pages technically works.

@scruffian
Copy link
Collaborator

There are a couple of open issues for pagination:
WordPress/gutenberg#26557
WordPress/gutenberg#24762

@carolinan
Copy link
Collaborator

Yes but they do not address why the block is not being used in the theme.

@scruffian
Copy link
Collaborator

I created an issue to add the block to this theme: #158

@kjellr
Copy link
Collaborator Author

kjellr commented Jan 6, 2021

Yes but they do not address why the block is not being used in the theme.

There's a simple answer actually: I tried adding it a while back, but it didn't seem to work. 😂

Let's look at sorting that out in another PR — maybe it's working now.

@carolinan carolinan merged commit 442cf33 into master Jan 7, 2021
@scruffian scruffian deleted the tt1/cleanup-for-release branch January 7, 2021 09:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants