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

Update the embed format. #59

Merged
merged 8 commits into from
Jun 3, 2022
Merged

Update the embed format. #59

merged 8 commits into from
Jun 3, 2022

Conversation

StevenDufresne
Copy link
Contributor

@StevenDufresne StevenDufresne commented Jun 2, 2022

Closes: #34

This PR updates the embed layout by creating an embed.php file and serving a custom embed view.

Changes:

  • If function add ()
  • If hook, apply the correct function that wraps it.
  • If hook of functions has arguments, add ... to denote parameters
    • I tried to show all the properties but it's overwhelming, at least how I had them styled.
  • Show parameters in a list
  • If parameters list is > 4 truncate

There are no examples of classes, and this wasn't tested on "classes" because of #35. This PR should probably wait for that bug fix.

If you think there is more pertinent information to add to the embed that is missing, please comment below!

Screenshots





@StevenDufresne StevenDufresne marked this pull request as ready for review June 2, 2022 04:32
@ryelle ryelle mentioned this pull request Jun 2, 2022
3 tasks
ryelle added 3 commits June 2, 2022 12:24
Increase contrast on text; move hook punctuation into the span (so it's also green); use a code font for the heading
@ryelle
Copy link
Contributor

ryelle commented Jun 2, 2022

This is looking good! I started playing with it locally so I hope you don't mind that I made a few changes to the CSS.

  • Increased the contrast of the text so it passes AA
  • Switched the title to use a code font, since it's the function/hook name
  • Moved the quotes and comma into the wp-embed-hook span, since those are also green on the main page
  • Switched to single quotes for hooks, unless there is a variable

Screen Shot 2022-06-02 at 12 32 27 PM

@StevenDufresne
Copy link
Contributor Author

Those are all great additions. What about we reduce the heading size to 20px? I find it kind of big, especially with the "Fira Code" font.

The difference appears negligible in the screenshots below, but it goes a long way for longer functions.

Before After

$title = get_the_title();
$has_args = count( get_params( $post_id ) ) > 0;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ryelle Why this change? I didn't want to make get_params a dependency of get_signature seeing that its values are not used. Although there are no tests, it does make it less testable.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oops, I left my reasoning in the commit, not the comment — I did that so it would match the main theme's template tag (e52a950). It also surprised me that you would need to tell get_signature that the function has args, it seems like something that should be detected in the function (since usually it would also output those params - though I know it doesn't 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.

Got it. I think I would prefer to have it less similar than more. Maybe it makes more sense to create a truncated form of the function in template-tags.php since it does appear like we'll be keeping the signature around as it is with some modifications.

What about something like:

function get_truncated_signature() {
 return get_signature( ..., true )
}

...

function get_signature( ..., truncate = false ) {

}

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, if we might use it elsewhere, I like your suggestion 👍🏻

@tellyworth
Copy link
Contributor

tellyworth commented Jun 3, 2022

These look great!

The hook stuff has got me thinking about the way we display those in the headline, ie apply_filters( 'hook', ... ) and do_action( 'hook', ... ). Not just in embeds but in general.

That is definitely clearer than displaying just the hook name. And it'll definitely make some people happy. But it feels kind of backwards to me, and I wonder if some other people might feel the same. What I mean is, in the function reference, the title/headline represents the code I should write in order to use the function, more or less. Whereas with the hooks, the headline represents the code core uses to call the hook, not the code I should write.

Perhaps both of these would be solved by having a one-line example immediately below the headline for every function, hook, etc. Something like:

apply_filters( 'pre_wp_filesize', ... )

add_filter( 'pre_wp_filesize', 'example_callback', 10, 2 );

..where the example line is syntax highlighted and easily copyable. We could also allow multiple examples, ref #12 (comment)

@dd32
Copy link
Member

dd32 commented Jun 3, 2022

Perhaps both of these would be solved by having a one-line example immediately below the headline for every function, hook
...
add_filter( 'pre_wp_filesize', 'example_callback', 10, 2 );

with #12 in mind, this would work well for hooks IMHO, but I'd expand that to have the function in there too:

add_filter( 'pre_wp_filesize', 'example_callback', 10, 2 );
function example_callback( $param1, $param2 ) {
    return $param1;
}

@ryelle ryelle merged commit d2f0c7b into trunk Jun 3, 2022
@ryelle ryelle deleted the try/update-embed branch June 3, 2022 16:26
@StevenDufresne
Copy link
Contributor Author

I agree that for hooks do_action & apply_filters are useless. Can we change the headline for those and then also follow up with a fully qualified example afterward?

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.

Improve function reference embeds
4 participants