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

Expand class-level documentation for WP_HTML_Tag_Processor #44478

Merged
merged 7 commits into from
Sep 30, 2022

Conversation

dmsnell
Copy link
Member

@dmsnell dmsnell commented Sep 26, 2022

What

Also fixes get_tag() to return upper-case variant of tag names in accordance with the inline documentation and with other DOM interfaces.

Why?

Because we want to help people understand how to use it, and get that information at the cursor where they're typing.

How?

Adds documentation in docblock comments.

Testing Instructions

The only thing to test is that get_tag() returns an upper-case tag name, which should be possible by inspection.

* Example
* ```php
* $processor = new WP_HTML_Tag_Processor( $html );
* if ( $processor->next_tag( array( 'tag_name' => 'option' ) ) ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about surfacing the shorthand form?

Suggested change
* if ( $processor->next_tag( array( 'tag_name' => 'option' ) ) ) {
* if ( $processor->next_tag( 'option' ) ) {

Copy link
Member Author

Choose a reason for hiding this comment

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

definitely not in this PR, but overall I'm not sure.

on one hand I think it's convenient, but on the other it does muddy the consistency, and I think it's likely more common that people will search by class anyway rather than tag, or at least reasonably as often as by class.

it did occur to me that maybe tag and class would suffice, instead of tag_name and class_name

on this note I wonder if we have to stick to the long array syntax in our example code. we have to use those in the code for PHP support, but are we required to demonstrate as examples the use of the more restricted syntax?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's likely more common that people will search by class anyway rather than tag, or at least reasonably as often as by class.

I'd say 50/50 is as good of a guess as we can make right now.

it did occur to me that maybe tag and class would suffice, instead of tag_name and class_name

Yes please!

are we required to demonstrate as examples the use of the more restricted syntax?

That is a good question, I don't know! @costdev what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

There's ~11 examples of short array syntax within docblocks/inline comments, and ~53 examples of long array syntax. While this example (see point 2) shows the long syntax, the accompanying text doesn't have anything to do with the "right" syntax to use. I'd suggest just going with best judgement on readability.

However, pinging @SergeyBiryukov as someone who has probably seen more Core docblocks than anyone.

Copy link
Member Author

@dmsnell dmsnell Sep 29, 2022

Choose a reason for hiding this comment

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

I've replaced the examples with shorthand array syntax for brevity and clarity. We can remove them later if we decide that it's best to leave in the array() notation.

31402ddfc8

* | Goal | Query |
* |-----------------------------------------------------------|------------------------------------------------------------------------------------|
* | Find any tag. | `$processor->next_tag();` |
* | Find next image tag. | `$processor->next_tag( array( 'tag_name' => 'img' ) );` |
Copy link
Contributor

@adamziel adamziel Sep 27, 2022

Choose a reason for hiding this comment

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

Let's mention the shorthand form here:

Suggested change
* | Find next image tag. | `$processor->next_tag( array( 'tag_name' => 'img' ) );` |
* | Find next image tag. | `$processor->next_tag( 'img' );` |
* | Find next image tag. | `$processor->next_tag( array( 'tag_name' => 'img' ) );` |

Copy link
Member Author

Choose a reason for hiding this comment

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

see my previous comment on this. I have second-guesses about this short-hand

Copy link
Contributor

@adamziel adamziel Sep 29, 2022

Choose a reason for hiding this comment

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

Wait, do you mean second thoughts on surfacing it here, or second thoughts on even supporting it?

Copy link
Member Author

Choose a reason for hiding this comment

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

supporting it at all. my worry is that we say you can search for a given tag name, but then people want a short hand for class name, so we add it, but then if you don't specify a tag you have ->next( null, 'wp-block-group' ) everywhere. or someone things, let's put class name first, and then we have the other problem.

I'd even be for exposing this shorthand only after we see use in practice to see if people even want it, and if so, which convention is more dominant.

alternatively it's always easy to later add new specialized methods. suppose we only expose ->next() right now, taking a query object. we can add ->next_tag( 'img' ) and ->next_with_class( 'wp-block-group' ). performance wise this is a "free" option

Copy link
Contributor

@adamziel adamziel Sep 30, 2022

Choose a reason for hiding this comment

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

I'd even be for exposing this shorthand only after we see use in practice to see if people even want it, and if so, which convention is more dominant.

Let's do that 👍 Renaming this method to just next( $query ) also works for me as next_tag( $tag_name ) makes a lot of sense.

*
* As with attribute values, adding or removing CSS classes is a safe
* operation that doesn't require checking if the attribute or class
* exists before making changes. If removing the only class then the
Copy link
Contributor

@adamziel adamziel Sep 27, 2022

Choose a reason for hiding this comment

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

Suggested change
* exists before making changes. If removing the only class then the
* exists before making changes. If removing the only class name, the

@adamziel
Copy link
Contributor

This is such a great documentation @dmsnell! 🎉 💯 🎉

I left some tactical notes, but overall this is great – thank you so much! ✅

@dmsnell dmsnell force-pushed the update/tag-processor-class-docs branch from be50156 to 9a50202 Compare September 28, 2022 18:12
@dmsnell
Copy link
Member Author

dmsnell commented Sep 28, 2022

Rebased from be50156 to 9a50202 with merge conflicts from trunk

dmsnell and others added 3 commits September 29, 2022 15:30
Also fixes `get_tag()` to return upper-case variant of tag names in
accordance with the inline documentation and with other DOM interfaces.
Author: @adamziel

Co-authored-by: Adam Zielinski <adam@adamziel.com>
@dmsnell dmsnell force-pushed the update/tag-processor-class-docs branch from 61d3f2b to ad72b63 Compare September 29, 2022 22:40
For consistency with other DOM libraries we're using upper-case tag names,
but we hadn't updated the code to reflect that.
@dmsnell dmsnell force-pushed the update/tag-processor-class-docs branch from a59f519 to aa746e6 Compare September 29, 2022 22:48
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.

3 participants