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

adds skip links check for block themes #457

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

MaggieCabrera
Copy link

@MaggieCabrera MaggieCabrera commented Sep 3, 2024

This PR adds a check only for block themes to see if templates have a main tag present. If they don't they will be missing the skip links from said template.

I need to check when a template is made out of a pattern and the main tag will be inside the pattern instead of the template file

@MaggieCabrera
Copy link
Author

We might want to check if there's multiple tags too

@MaggieCabrera MaggieCabrera marked this pull request as draft September 3, 2024 11:05
@MaggieCabrera MaggieCabrera marked this pull request as ready for review September 5, 2024 15:10
Copy link
Collaborator

@matiasbenedetto matiasbenedetto left a comment

Choose a reason for hiding this comment

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

the <main> tag is being rendered in the HTML instead of being printed as text:

image

@matiasbenedetto
Copy link
Collaborator

It would be nice to make the main tag search recusive for nested patterns.

@MaggieCabrera
Copy link
Author

I fixed those! ready for another review

Co-authored-by: Matias Benedetto <matias.benedetto@gmail.com>
return false;
}

$files = glob( $theme_dir . '/' . $directory . '/*.php' );
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as in #457 (comment)

Copy link
Author

Choose a reason for hiding this comment

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

It looks like $php_files doesn't store the contents of the comment in the pattern, so I have no way of checking if the slug of the pattern is the one I'm trying to match

Copy link
Author

Choose a reason for hiding this comment

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

seems like that's done here so I will leave this part of the code as I implemented it:

$php[ $filename ] = tc_strip_comments( $php[ $filename ] );

@kafleg
Copy link
Member

kafleg commented Sep 17, 2024

I checked by removing "tagName":"main" from the template files in Twenty Twenty Four theme but this PR won't show anything.

Would you please let me know how did you test that?

@kafleg
Copy link
Member

kafleg commented Sep 17, 2024

I checked by removing "tagName":"main" from the template files in Twenty Twenty Four theme but this PR won't show anything.

Would you please let me know how did you test that?

I find it. I was checking different PR. Sorry!

@kafleg
Copy link
Member

kafleg commented Sep 17, 2024

REQUIRED Skip links are missing from the following templates: single.html, single.html Please make sure the templates have a

tag.

Why the message show single.html, single.html twice?

@MaggieCabrera
Copy link
Author

REQUIRED Skip links are missing from the following templates: single.html, single.html Please make sure the templates have a
tag.

Why the message show single.html, single.html twice?

I haven't seen that on my tests, what did you try to get that to happen?

@kafleg
Copy link
Member

kafleg commented Sep 17, 2024

In single.html file of TT4 theme, I simply changed <main class="wp-block-group alignfull"> to <div class="wp-block-group alignfull">

@kafleg
Copy link
Member

kafleg commented Sep 17, 2024

I think I found the reason,
I changed on index.html and single.html in two files and it is happended.

@MaggieCabrera
Copy link
Author

I think I found the reason, I changed on index.html and single.html in two files and it is happended.

I just fixed this

@MaggieCabrera
Copy link
Author

Sorry, I have been super busy; I just caught up with the reviews. It should be ready to go now!

Comment on lines +19 to +37
/**
* Returns true if the theme is a block theme.
*
* @var array $is_block_theme
*/
protected $is_block_theme = false;

/**
* The WP_Theme instance being checked.
*
* @var WP_Theme $wp_theme
*/
protected $wp_theme = false;

function set_context( $data ) {
if ( isset( $data['theme'] ) ) {
$this->wp_theme = $data['theme'];
$this->is_block_theme = wp_is_block_theme();
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

As far as I can see is_block_theme is not being used in this check class. Is it used somewhere?

Copy link
Collaborator

@matiasbenedetto matiasbenedetto left a comment

Choose a reason for hiding this comment

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

I think the Skip_Links_Check class needs to be added to the list of FSE specific checks here:

theme-check/checkbase.php

Lines 386 to 387 in dabaf1d

// Add FSE specific checks.
$themechecks[] = new FSE_Required_Files_Check();

So this line needs to be removed:

$themechecks[] = new Skip_Links_Check();

See FSE_Required_Files_Check class as reference.

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

Successfully merging this pull request may close these issues.

3 participants