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

gin0115 · 2023-07-04T12:57:38Z

Wraps the outputted links in ...

mentions #5

georgestephanis

I'm hard opposed to this, as that could add extra markup -- that is a block-level element -- across a lot of sites.

What I'd be open to is passing the string through a filter before echoing it, so you can add a wrapper on a specific site if you'd like via add_filter()

georgestephanis · 2023-07-06T14:21:15Z

This is meant to be a drop in replacement for existing markup across all sites. Shoving an extra block-level wrapper around it could cause unintended visual breakage on some sites that already have Colophon in place.

…on. Includes an additional fitler, which allows the whole rendered string to be controlled programaiticaly

…mpty span tag. Also added an additional param to the filter to pass the rendered links to the filter

gin0115 · 2023-07-07T08:19:54Z

Additional changes have been made.

passing wrapper='' will see the links wrapped in an empty span tag
.....

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


<!-- Renders as -->
<span>LINKS</span>

Passing a value will see them wrapped in a span with the passed values as the class wrapper='colophon__wrapper
...

<!-- wp:shortcode -->
[team51-credits wrapper="bar" /]
<!-- /wp:shortcode -->


<!-- Renders as -->
<span class="bar">LINKS</span>

There is also a new filter

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:

/**
 * 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 );

<!-- wp:shortcode -->
[team51-credits wrapper="foo" /]
<!-- /wp:shortcode -->


<!-- Renders as -->
<div class="colophon"><span class="foo">LINKS</span></div>

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

/**
 * 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 );

<!-- wp:shortcode -->
[team51-credits wrapper="ignore-me" /]
<!-- /wp:shortcode -->


<!-- Renders as -->
<div class="some-class"><p id="colophon-wrapper">LINKS</p></div>

The above would output the same, even if wrapper="" was not passed. [team51-credits /]

gin0115

This has now been changed, please see the comments in the rest of this PR

gin0115 · 2023-12-19T11:14:36Z

To clarify above

These changes will not see any changes to existing sites that are using it. You can now pass the wrapper arg

<!-- wp:shortcode -->
[team51-credits wrapper="bar" /]
<!-- /wp:shortcode -->


<!-- Renders as -->
<span class="bar">LINKS</span>

You can also use the filters

georgestephanis · 2023-12-19T15:30:22Z

colophon.php

@@ -20,7 +20,7 @@
 	 *
 	 * @return void
 	 */
-	function team51_credits( $args = array() ) {
+	function team51_credits( $args = array() ): void {


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.

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!)

georgestephanis

It feels like we're trying to do both customization (with the new broad filter on the output) and configuration (with the new wrapper parameter) -- rather than choosing one. I'm not sure I agree with the rationale of adding two ways in to accomplish this, versus choosing a method and committing to it.

Very open to input or context as I've been somewhat out of the loop on this.

georgestephanis · 2023-12-19T15:43:21Z

colophon.php

-		$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';


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.

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?

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

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

georgestephanis · 2023-12-19T15:47:15Z

colophon.php

-		team51_credits( $atts );
-		return ob_get_clean();
+		team51_credits( $attributes );
+		return ob_get_clean() ?: ''; // phpcs:ignore Universal.Operators.DisallowShortTernary.Found


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?)

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

georgestephanis · 2023-12-19T15:53:48Z

colophon.php

+			$wrapped_template = '' !== $args['wrapper']
+				? '<span class="%1$s">%2$s</span>'
+				: '<span>%2$s</span>';


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']

? '%2$s'

: '%2$s';

$wrapped_template = '%2$s';

// Otherwise, include it as a class name...

if ( $args['wrapper'] ) {

$wrapped_template = '%2$s';

}

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

… if no wrapepr class passed.

gin0115 · 2024-04-25T11:45:57Z

Hey @georgestephanis

This has been a while, I have implemented all the changes above including some additional ones.

I have removed the filter which allowed for the filtering of the final output and updated the docs to reflect this.

Did you want me to close this and open a new PR?

In a nutshell, this now allows you to wrap the links in  tags with optional wrappers.
By default there is no

or s added, unless included (so backwards compatible) and these are all editable via the block which has been included.

gin0115 · 2024-04-25T11:46:32Z

Suspect we are better closing this as the conflict on readme, isnt present in the code and is as a result of further changes

Added p tag with colophon__wrapper as the class around links

36df3e1

gin0115 linked an issue Jul 4, 2023 that may be closed by this pull request

Add  tag around shortcode output #5

Open

gin0115 requested a review from georgestephanis July 6, 2023 14:00

georgestephanis requested changes Jul 6, 2023

View reviewed changes

gin0115 added 3 commits July 6, 2023 16:53

Merge branch 'trunk' into feature/gh5-wrap-tag-output-between-p-tags

a8edc62

Make the wrapper an arg which can be passed when rendering the coloph…

1a83ee5

…on. Includes an additional fitler, which allows the whole rendered string to be controlled programaiticaly

Remove the default class, if no class is passed will now wrap in an e…

32c3408

…mpty span tag. Also added an additional param to the filter to pass the rendered links to the filter

Fixed all phpcs errors and added types to function signatures

9c0904c

gin0115 commented Dec 19, 2023

View reviewed changes

georgestephanis reviewed Dec 19, 2023

View reviewed changes

georgestephanis requested changes Dec 19, 2023

View reviewed changes

Remove pointless check in render function which allowed an empty span…

3f27aa6

… if no wrapepr class passed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

gin0115 commented Jul 4, 2023

georgestephanis left a comment

georgestephanis commented Jul 6, 2023

gin0115 commented Jul 7, 2023

gin0115 left a comment

gin0115 commented Dec 19, 2023

georgestephanis Dec 19, 2023

gin0115 Dec 19, 2023

georgestephanis left a comment

georgestephanis Dec 19, 2023

georgestephanis Dec 19, 2023

gin0115 Dec 19, 2023

gin0115 Dec 19, 2023

georgestephanis Dec 19, 2023

gin0115 Dec 19, 2023

georgestephanis Dec 19, 2023

gin0115 Dec 19, 2023

gin0115 commented Apr 25, 2024

gin0115 commented Apr 25, 2024

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

-			$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>';
+			}

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

Are you sure you want to change the base?

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

Conversation

gin0115 commented Jul 4, 2023

georgestephanis left a comment

Choose a reason for hiding this comment

georgestephanis commented Jul 6, 2023

gin0115 commented Jul 7, 2023

gin0115 left a comment

Choose a reason for hiding this comment

gin0115 commented Dec 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

georgestephanis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gin0115 commented Apr 25, 2024

gin0115 commented Apr 25, 2024