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.

Sign up for GitHub

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

Jump to bottom

Added p tag with colophon__wrapper as the class around links #13

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

Added p tag with colophon__wrapper as the class around links #13

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
36df3e1
Added p tag with colophon__wrapper as the class around links
gin0115 Jul 4, 2023
a8edc62
Merge branch 'trunk' into feature/gh5-wrap-tag-output-between-p-tags
gin0115 Jul 6, 2023
1a83ee5
Make the wrapper an arg which can be passed when rendering the coloph…
gin0115 Jul 6, 2023
32c3408
Remove the default class, if no class is passed will now wrap in an e…
gin0115 Jul 7, 2023
9c0904c
Fixed all phpcs errors and added types to function signatures
gin0115 Dec 19, 2023
3f27aa6
Remove pointless check in render function which allowed an empty span…
gin0115 Dec 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 42 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ Parameters can be passed in --
* `pressable` -- The text displayed for the backlink to Pressable
Defaults to `Hosted by Pressable.`
Link is skipped if not truthy.
* `wrapper` -- Wraps the links in a `<span>` with the given class. Passing a blank string `[team51_credits wrapper=""]` will see `colophon__wrapper` used.

Customization
=============
Expand All @@ -47,6 +48,40 @@ function PREFIX_team51_credit_links( $credit_links ) {
add_filter( 'team51_credit_links', 'PREFIX_team51_credit_links' );
```

An additional filter has been added which is used to modify the final output of the colophon. This can be used to add additional markup around the colophon, or to modify the output of the colophon itself. It can be done like so:

```php
/**
* Wraps the colophon in a div with a class of `colophon`.
*
* @param $output (string) The output of the colophon.
* @param $links (string) The unwrapped links string.
* @param $args (array) The arguments passed to the colophon.
* @return (string) The output of the colophon, wrapped in a div.
*/
function PREFIX_team51_wrap_colophon( $output, $links, $args ) {
return '<div class="colophon">' . $output . '</div>';
}
add_filter( 'team51_credits_render', 'PREFIX_team51_wrap_colophon', 10, 3 );
```

You can also choose to wrap the links in any tags desired:

```php
/**
* Wraps the colophon in a div with a class of `colophon`.
*
* @param $output (string) The output of the colophon.
* @param $links (string) The unwrapped links string.
* @param $args (array) The arguments passed to the colophon.
* @return (string) The output of the colophon, wrapped in a div.
*/
function PREFIX_team51_colophon_as_paragraph( $output, $links, $args ) {
return '<div class="some-class"><p id="colophon-wrapper">' . $links . '</p></div>';
}
add_filter( 'team51_credits_render', 'PREFIX_team51_colophon_as_paragraph', 10, 3 );
```

FSE Themes
==========

Expand Down Expand Up @@ -92,6 +127,13 @@ Change WordPress link text to be “Proudly designed with WordPress”:
<!-- /wp:shortcode -->
```

With a wrapper (span with class of `colophon__wrapper`):

```html
<!-- wp:shortcode -->
[team51-credits wrapper="colophon__wrapper" /]
<!-- /wp:shortcode -->
```


Installation
Expand Down
49 changes: 39 additions & 10 deletions colophon.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
*
* @return void
*/
function team51_credits( $args = array() ) {
function team51_credits( $args = array() ): void {
Copy link
Contributor

Choose a reason for hiding this comment

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

It should be fine with the return type declaration here but just acknowledging that mandates php7.

Which I don't think should be a problem, my brain's just always touchy on making sure such things are explicit, having survived the PHP 5.2 and 5.3 support wars back in the day.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The function only echos, there is no return, so if someone were to add a return and not change the definition, it would prompt them in development to look at why/what.

(Don't say things like this, it reminds many of us of how long we've been around, I still carry around some PHP4 scars, ha!)

$args = wp_parse_args(
$args,
array(
Expand Down Expand Up @@ -84,13 +84,38 @@ function team51_credits( $args = array() ) {
*
* @param array $credit_links The associative array of credit links.
* @param array $args The parsed arguments used by `team51_credits()`.
*
* @return array The filtered associative array of credit links.
*/
$credit_links = apply_filters( 'team51_credit_links', $credit_links, $args );

echo implode(
esc_html( $args['separator'] ),
$credit_links //phpcs:ignore WordPress.Security.EscapeOutput.OutputNotEscaped, this cant be escaped as it runs through a filter
);
$links = implode( esc_html( $args['separator'] ), $credit_links );

// If we have an args['wrapper'] set, wrap the output in that.
if ( array_key_exists( 'wrapper', $args ) ) {
// If there is no class defined, use an empty span.
$wrapped_template = '' !== $args['wrapper']
? '<span class="%1$s">%2$s</span>'
: '<span>%2$s</span>';
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd far sooner see this written out, rather than doing assignments, comparisons, and ternaries on a single line.

Suggested change
$wrapped_template = '' !== $args['wrapper']
? '<span class="%1$s">%2$s</span>'
: '<span>%2$s</span>';
$wrapped_template = '<span>%2$s</span>';
// Otherwise, include it as a class name...
if ( $args['wrapper'] ) {
$wrapped_template = '<span class="%1$s">%2$s</span>';
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this has been removed as the args are passed through the shortcode handler by default.


$string = sprintf( $wrapped_template, esc_attr( $args['wrapper'] ), $links );
} else {
// Otherwise, just return the links.
$string = $links;
}

/**
* Filters the output string.
*
* @param string $string The output string.
* @param string $links The unwrapped links string.
* @param array<string, mixed> $args The parsed arguments used by `team51_credits()`.
*
* @return string The filtered output string.
*/
$string = apply_filters( 'team51_credits_render', $string, $links, $args );

echo $string; // phpcs:ignore WordPress.Security.EscapeOutput.OutputNotEscaped, this cant be escaped as its filtered.
}
add_action( 'team51_credits', 'team51_credits', 10, 1 );
endif;
Expand All @@ -102,11 +127,11 @@ function team51_credits( $args = array() ) {
*
* Can also be used in the Shortcode block.
*
* @param array{separator?: string, wpcom?: string, pressable?: string} $atts The Args passed to the function.
* @param array{separator?: string, wpcom?: string, pressable?: string} $attributes The Args passed to the function.
*
* @return string
*/
function team51_credits_shortcode( $atts ) {
function team51_credits_shortcode( array $attributes = array() ): string {
$pairs = array(
'separator' => ' ',
/* translators: %s: WordPress. */
Expand All @@ -115,11 +140,15 @@ function team51_credits_shortcode( $atts ) {
'pressable' => sprintf( __( 'Hosted by %s.', 'team51' ), 'Pressable' ),
);

$atts = shortcode_atts( $pairs, $atts, 'team51-credits' );
// Add the wrapper if set.
if ( is_array( $attributes ) && array_key_exists( 'wrapper', $attributes ) ) {
$pairs['wrapper'] = '' !== $attributes['wrapper'] ? esc_attr( $attributes['wrapper'] ) : 'colophon__wrapper';
Copy link
Contributor

Choose a reason for hiding this comment

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

For legibility and ability to understand what's going on here at a glance, I'd far sooner see this broken out to multiple lines rather than mixing ternaries and assignments and conditionals on one line.

			$pairs['wrapper'] = 'colophon__wrapper';
			if ( $attributes['wrapper'] ) {
				$pairs['wrapper'] = esc_attr( $attributes['wrapper'] );
			}

would be a direct rewriting, however as $pairs and $attributes both get passed through shortcode_atts() -- so long as the default is being added to $pairs, I believe that shortcode_atts() should handle the splicing in of whatever's in $attributes -- so I'd prefer to just modify it in $attributes if it's empty -- to let the default pass through, giving us something closer to --

			$pairs['wrapper'] = 'colophon__wrapper';
			if ( empty( $attributes['wrapper'] ) ) {
				unset( $attributes['wrapper'] );
			}

or

Suggested change
$pairs['wrapper'] = '' !== $attributes['wrapper'] ? esc_attr( $attributes['wrapper'] ) : 'colophon__wrapper';
$pairs['wrapper'] = 'colophon__wrapper';
if ( empty( $attributes['wrapper'] ) ) {
$attributes['wrapper'] = 'colophon__wrapper';
}

depending on what we'd like it to look like if anyone tries to hook into the subsequent shortcode_atts_{$shortcode} filter inside shortcode_atts() or the like.

Copy link
Contributor

Choose a reason for hiding this comment

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

Reading how this is then handled inside the new team51_credits(); I'm not sure I've got the gist right in my suggestions. If someone passes in a blank string, some remarks above indicate that colophon_wrapper will be used -- however the team51_credits has a contingency for dropping the class attribute from the wrapper span when passed in as empty -- which the shortcode prevents from happening as-is. I'd like to check on the intent of expectations here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So with the above, if you do not pass wrapper as an attribute the span should not be added (this means existing sites do not see any changes. Adding the default wrapper class to $pairs sees the wrapped added.

So this is why we check if it exists, then its blank we add the fallback, else we use what's passed.

There are 3 possible combinations here.

You are correct that the check if the wrapp is empty and not rendering the class is pointless as you cant pass an empty class. So i can remove that

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

as for the filters, these just allow you to change it from being wrapped in a span to you being able to wrap it in a div or as a table. The same with the links, the filter just allows you to add additional tracking params or remove them.

Was not asked for in the brief but felt they added additional flexibility

}

$attributes = shortcode_atts( $pairs, $attributes, 'team51-credits' );
ob_start();
team51_credits( $atts );
return ob_get_clean();
team51_credits( $attributes );
return ob_get_clean() ?: ''; // phpcs:ignore Universal.Operators.DisallowShortTernary.Found
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe that the only scenario where ob_get_clean() returns false -- or a non-string -- is when out buffering isn't active? And as we're starting it two lines earlier, I don't think that's a risk.

Is there a scenario where we're expecting it would return null or false or something falsey that we need to convert to an empty string? (And if so, I wonder if casting to a string may be simpler?)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, that's just a habit of mine, ide flags it as a possibility and i just account for it.
Removed

}
add_action(
'init',
Expand Down