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

Marketing Notices for Black Friday. #2225

Merged
merged 58 commits into from
Oct 15, 2024
Merged

Conversation

bordoni
Copy link
Member

@bordoni bordoni commented Oct 11, 2024

🎫 Ticket

ET-2241 TEC-5242

🗒️ Description

As you review this, please make sure you keep in mind this is for a Black Friday.

Still haven't touched the existing tests here, so I am expecting them to break, will fix them tomorrow.

🎥 Artifacts

https://i.bordoni.me/GqsJbYmj

--

Video update on Oct 14 with dismissal:
https://i.bordoni.me/XKC3GDY0

@bordoni bordoni added the code review Status: requires a code review. label Oct 11, 2024
@bordoni bordoni self-assigned this Oct 11, 2024
Copy link
Contributor

@tec-bot tec-bot left a comment

Choose a reason for hiding this comment

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

Remaining comments which cannot be posted as a review comment to avoid GitHub Rate Limit

phpcs

[phpcs] reported by reviewdog 🐶
Generic.WhiteSpace.DisallowSpaceIndent.SpacesUsed
Tabs must be used to indent lines; spaces are not allowed


[phpcs] reported by reviewdog 🐶
Generic.WhiteSpace.DisallowSpaceIndent.SpacesUsed
Tabs must be used to indent lines; spaces are not allowed


[phpcs] reported by reviewdog 🐶
Generic.WhiteSpace.DisallowSpaceIndent.SpacesUsed
Tabs must be used to indent lines; spaces are not allowed

array_unshift( $this->sections, $section );


[phpcs] reported by reviewdog 🐶
Squiz.Commenting.FunctionComment.InvalidReturnVoid
Function return type is void, but function contains return statement


[phpcs] reported by reviewdog 🐶
Generic.WhiteSpace.DisallowSpaceIndent.SpacesUsed
Tabs must be used to indent lines; spaces are not allowed


[phpcs] reported by reviewdog 🐶
Generic.WhiteSpace.DisallowSpaceIndent.SpacesUsed
Tabs must be used to indent lines; spaces are not allowed

<div class="tec-settings-form__sidebar-section tec-settings-form__sidebar-header">


[phpcs] reported by reviewdog 🐶
Generic.WhiteSpace.DisallowSpaceIndent.SpacesUsed
Tabs must be used to indent lines; spaces are not allowed

<?php do_action( 'tec_settings_sidebar_header_start' ); ?>


[phpcs] reported by reviewdog 🐶
Generic.WhiteSpace.DisallowSpaceIndent.SpacesUsed
Tabs must be used to indent lines; spaces are not allowed


[phpcs] reported by reviewdog 🐶
Generic.WhiteSpace.DisallowSpaceIndent.SpacesUsed
Tabs must be used to indent lines; spaces are not allowed

$this->render_header_image();


[phpcs] reported by reviewdog 🐶
Generic.WhiteSpace.DisallowSpaceIndent.SpacesUsed
Tabs must be used to indent lines; spaces are not allowed


[phpcs] reported by reviewdog 🐶
Generic.WhiteSpace.DisallowSpaceIndent.SpacesUsed
Tabs must be used to indent lines; spaces are not allowed


[phpcs] reported by reviewdog 🐶
Generic.WhiteSpace.DisallowSpaceIndent.SpacesUsed
Tabs must be used to indent lines; spaces are not allowed

<?php do_action( 'tec_settings_sidebar_header_end' ); ?>


[phpcs] reported by reviewdog 🐶
Generic.WhiteSpace.DisallowSpaceIndent.SpacesUsed
Tabs must be used to indent lines; spaces are not allowed


[phpcs] reported by reviewdog 🐶
Generic.WhiteSpace.DisallowSpaceIndent.SpacesUsed
Tabs must be used to indent lines; spaces are not allowed

<?php foreach ( $this->sections as $section ) : ?>


[phpcs] reported by reviewdog 🐶
Generic.WhiteSpace.DisallowSpaceIndent.SpacesUsed
Tabs must be used to indent lines; spaces are not allowed

<div class="tec-settings-form__sidebar-section">


[phpcs] reported by reviewdog 🐶
Generic.WhiteSpace.DisallowSpaceIndent.SpacesUsed
Tabs must be used to indent lines; spaces are not allowed


[phpcs] reported by reviewdog 🐶
Generic.WhiteSpace.DisallowSpaceIndent.SpacesUsed
Tabs must be used to indent lines; spaces are not allowed


[phpcs] reported by reviewdog 🐶
Generic.WhiteSpace.DisallowSpaceIndent.SpacesUsed
Tabs must be used to indent lines; spaces are not allowed


[phpcs] reported by reviewdog 🐶
Generic.WhiteSpace.DisallowSpaceIndent.SpacesUsed
Tabs must be used to indent lines; spaces are not allowed

<?php tribe( \TEC\Common\Admin\Conditional_Content\Black_Friday::class )->render_wide_banner_html(); ?>


[phpcs] reported by reviewdog 🐶
WordPress.WP.GlobalVariablesOverride.Prohibited
Overriding WordPress globals is prohibited. Found assignment to $year


[phpcs] reported by reviewdog 🐶
Generic.WhiteSpace.DisallowSpaceIndent.SpacesUsed
Tabs must be used to indent lines; spaces are not allowed

href="<?php echo esc_url( $link ); ?>"


[phpcs] reported by reviewdog 🐶
Generic.WhiteSpace.DisallowSpaceIndent.SpacesUsed
Tabs must be used to indent lines; spaces are not allowed


[phpcs] reported by reviewdog 🐶
Generic.WhiteSpace.DisallowSpaceIndent.SpacesUsed
Tabs must be used to indent lines; spaces are not allowed


[phpcs] reported by reviewdog 🐶
Generic.WhiteSpace.DisallowSpaceIndent.SpacesUsed
Tabs must be used to indent lines; spaces are not allowed

style="display: block; margin: 25px 0; <?php echo $this->get( 'is_narrow' ) ? 'max-width: 1000px' : ''; ?>"


[phpcs] reported by reviewdog 🐶
Generic.WhiteSpace.DisallowSpaceIndent.SpacesUsed
Tabs must be used to indent lines; spaces are not allowed

title="<?php echo sprintf( esc_attr_x( '%1$s Black Friday Sale for The Events Calendar plugins, add-ons and bundles.', 'Alt text for the Black Friday Ad', 'tribe-common' ), $year ); ?>"


[phpcs] reported by reviewdog 🐶
Universal.CodeAnalysis.NoEchoSprintf.Found
Unnecessary "echo sprintf(...)" found. Use "printf(...)" instead.

title="<?php echo sprintf( esc_attr_x( '%1$s Black Friday Sale for The Events Calendar plugins, add-ons and bundles.', 'Alt text for the Black Friday Ad', 'tribe-common' ), $year ); ?>"


[phpcs] reported by reviewdog 🐶
WordPress.WP.I18n.MissingTranslatorsComment
A function call to esc_attr_x() with texts containing placeholders was found, but was not accompanied by a "translators:" comment on the line above to clarify the meaning of the placeholders.

title="<?php echo sprintf( esc_attr_x( '%1$s Black Friday Sale for The Events Calendar plugins, add-ons and bundles.', 'Alt text for the Black Friday Ad', 'tribe-common' ), $year ); ?>"


[phpcs] reported by reviewdog 🐶
StellarWP.XSS.EscapeOutput.OutputNotEscaped
Expected next thing to be an escaping function (see Codex for 'Data Validation'), not '$year'

title="<?php echo sprintf( esc_attr_x( '%1$s Black Friday Sale for The Events Calendar plugins, add-ons and bundles.', 'Alt text for the Black Friday Ad', 'tribe-common' ), $year ); ?>"


[phpcs] reported by reviewdog 🐶
WordPress.Security.EscapeOutput.OutputNotEscaped
All output should be run through an escaping function (see the Security sections in the WordPress Developer Handbooks), found '$year'.

title="<?php echo sprintf( esc_attr_x( '%1$s Black Friday Sale for The Events Calendar plugins, add-ons and bundles.', 'Alt text for the Black Friday Ad', 'tribe-common' ), $year ); ?>"


[phpcs] reported by reviewdog 🐶
Generic.WhiteSpace.DisallowSpaceIndent.SpacesUsed
Tabs must be used to indent lines; spaces are not allowed

style="display: block; width: 100%; height: auto;" <?php // This is intentionally inline, don't add classes here. ?>


[phpcs] reported by reviewdog 🐶
Generic.WhiteSpace.DisallowSpaceIndent.SpacesUsed
Tabs must be used to indent lines; spaces are not allowed

src="<?php echo esc_url( $image_src ); ?>"


[phpcs] reported by reviewdog 🐶
Generic.WhiteSpace.DisallowSpaceIndent.SpacesUsed
Tabs must be used to indent lines; spaces are not allowed

alt="<?php echo sprintf( esc_attr_x( '%1$s Black Friday Sale for The Events Calendar plugins, add-ons and bundles.', 'Alt text for the Black Friday Ad', 'tribe-common' ), $year ); ?>"


[phpcs] reported by reviewdog 🐶
Universal.CodeAnalysis.NoEchoSprintf.Found
Unnecessary "echo sprintf(...)" found. Use "printf(...)" instead.

alt="<?php echo sprintf( esc_attr_x( '%1$s Black Friday Sale for The Events Calendar plugins, add-ons and bundles.', 'Alt text for the Black Friday Ad', 'tribe-common' ), $year ); ?>"


[phpcs] reported by reviewdog 🐶
WordPress.WP.I18n.MissingTranslatorsComment
A function call to esc_attr_x() with texts containing placeholders was found, but was not accompanied by a "translators:" comment on the line above to clarify the meaning of the placeholders.

alt="<?php echo sprintf( esc_attr_x( '%1$s Black Friday Sale for The Events Calendar plugins, add-ons and bundles.', 'Alt text for the Black Friday Ad', 'tribe-common' ), $year ); ?>"


[phpcs] reported by reviewdog 🐶
StellarWP.XSS.EscapeOutput.OutputNotEscaped
Expected next thing to be an escaping function (see Codex for 'Data Validation'), not '$year'

alt="<?php echo sprintf( esc_attr_x( '%1$s Black Friday Sale for The Events Calendar plugins, add-ons and bundles.', 'Alt text for the Black Friday Ad', 'tribe-common' ), $year ); ?>"


[phpcs] reported by reviewdog 🐶
WordPress.Security.EscapeOutput.OutputNotEscaped
All output should be run through an escaping function (see the Security sections in the WordPress Developer Handbooks), found '$year'.

alt="<?php echo sprintf( esc_attr_x( '%1$s Black Friday Sale for The Events Calendar plugins, add-ons and bundles.', 'Alt text for the Black Friday Ad', 'tribe-common' ), $year ); ?>"


[phpcs] reported by reviewdog 🐶
Generic.WhiteSpace.DisallowSpaceIndent.SpacesUsed
Tabs must be used to indent lines; spaces are not allowed

src/Common/Admin/Entities/Link.php Outdated Show resolved Hide resolved
Copy link
Contributor

@tec-bot tec-bot left a comment

Choose a reason for hiding this comment

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

Remaining comments which cannot be posted as a review comment to avoid GitHub Rate Limit

phpcs

[phpcs] reported by reviewdog 🐶
WordPress.WP.I18n.MissingTranslatorsComment
A function call to esc_attr_x() with texts containing placeholders was found, but was not accompanied by a "translators:" comment on the line above to clarify the meaning of the placeholders.

alt="<?php echo sprintf( esc_attr_x( '%1$s Black Friday Sale for The Events Calendar plugins, add-ons and bundles.', 'Alt text for the Black Friday Ad', 'tribe-common' ), $year ); ?>"


[phpcs] reported by reviewdog 🐶
StellarWP.XSS.EscapeOutput.OutputNotEscaped
Expected next thing to be an escaping function (see Codex for 'Data Validation'), not '$year'

alt="<?php echo sprintf( esc_attr_x( '%1$s Black Friday Sale for The Events Calendar plugins, add-ons and bundles.', 'Alt text for the Black Friday Ad', 'tribe-common' ), $year ); ?>"


[phpcs] reported by reviewdog 🐶
WordPress.Security.EscapeOutput.OutputNotEscaped
All output should be run through an escaping function (see the Security sections in the WordPress Developer Handbooks), found '$year'.

alt="<?php echo sprintf( esc_attr_x( '%1$s Black Friday Sale for The Events Calendar plugins, add-ons and bundles.', 'Alt text for the Black Friday Ad', 'tribe-common' ), $year ); ?>"


[phpcs] reported by reviewdog 🐶
Generic.WhiteSpace.DisallowSpaceIndent.SpacesUsed
Tabs must be used to indent lines; spaces are not allowed

@the-events-calendar the-events-calendar deleted a comment from tec-bot Oct 11, 2024
@the-events-calendar the-events-calendar deleted a comment from tec-bot Oct 11, 2024
@the-events-calendar the-events-calendar deleted a comment from tec-bot Oct 11, 2024
[
'style' => 'position: absolute; top: 0; right: 0; background: transparent; border: 0; color: #fff; padding: 0.5em; cursor: pointer;',
'data-tec-conditional-content-dismiss-button' => true,
'data-tec-conditional-content-dismiss-slug' => $this->slug,
Copy link
Contributor

Choose a reason for hiding this comment

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

[phpcs] reported by reviewdog 🐶
WordPress.Arrays.MultipleStatementAlignment.LongIndexSpaceBeforeDoubleArrow
Expected 1 space between "'data-tec-conditional-content-dismiss-slug'" and double arrow; 3 found.

Copy link
Member

Choose a reason for hiding this comment

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

I've seen this too - apparently it breaks if one key is above some length. I haven't tested to find the "sweet spot"

Copy link
Contributor

@JPry JPry left a comment

Choose a reason for hiding this comment

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

The things I commented on before are looking good, with a few minor suggestions.

I made a few comments on some of the new stuff that was added, and most of it is also minor.

One thing I would also suggest for the Dismissible_Trait: I would recommend having some kind of validation method that handles common situations that would trigger a situation where we can't continue (like empty( $this->slug ) or empty( tribe_get_request_var( 'slug', false ) ), etc.). This method can throw an exception if there's an error, and silently pass if there's not an error. Then the calling code wraps that check in a try/catch block, and any exception just triggers the false response.

Let me know if you'd like to see an example of what I mean.

src/Common/Admin/Entities/Button.php Outdated Show resolved Hide resolved
src/Common/Admin/Entities/Link.php Outdated Show resolved Hide resolved
src/Common/Admin/Entities/Link.php Outdated Show resolved Hide resolved
src/Common/Admin/Settings_Sidebar.php Outdated Show resolved Hide resolved
src/Common/Admin/Settings_Sidebar_Section.php Outdated Show resolved Hide resolved
src/Common/Admin/Conditional_Content/Dismissible_Trait.php Outdated Show resolved Hide resolved
return true;
}

update_user_meta( $user_id, $this->meta_key_time_prefix . $this->slug, time() );
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar to how the nonce_action_prefix value is combined with slug, I think it would make sense to have a helper method to combine the two values and return the real meta key. Something like get_time_meta_key().

src/Common/Admin/Conditional_Content/Dismissible_Trait.php Outdated Show resolved Hide resolved
src/Common/Admin/Conditional_Content/Dismissible_Trait.php Outdated Show resolved Hide resolved
src/Common/Admin/Conditional_Content/Dismissible_Trait.php Outdated Show resolved Hide resolved
@@ -0,0 +1,4 @@
Significance: minor
Copy link
Member

Choose a reason for hiding this comment

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

Should there be another of these? This only covers the deprecations.

package.json Show resolved Hide resolved
src/Common/Admin/Conditional_Content/Black_Friday.php Outdated Show resolved Hide resolved
src/Common/Admin/Conditional_Content/Black_Friday.php Outdated Show resolved Hide resolved
[
'style' => 'position: absolute; top: 0; right: 0; background: transparent; border: 0; color: #fff; padding: 0.5em; cursor: pointer;',
'data-tec-conditional-content-dismiss-button' => true,
'data-tec-conditional-content-dismiss-slug' => $this->slug,
Copy link
Member

Choose a reason for hiding this comment

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

I've seen this too - apparently it breaks if one key is above some length. I haven't tested to find the "sweet spot"

*/

$year = date_i18n( 'Y' );
Copy link
Member

Choose a reason for hiding this comment

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

Use $sale_year if you care

bordoni and others added 2 commits October 15, 2024 14:37
Copy link
Contributor

@tec-bot tec-bot left a comment

Choose a reason for hiding this comment

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

Remaining comments which cannot be posted as a review comment to avoid GitHub Rate Limit

phpcs

[phpcs] reported by reviewdog 🐶
Generic.WhiteSpace.DisallowSpaceIndent.SpacesUsed
Tabs must be used to indent lines; spaces are not allowed


[phpcs] reported by reviewdog 🐶
Squiz.Commenting.DocCommentAlignment.SpaceBeforeStar
Expected 1 space before asterisk; 5 found

return;
}

$this->container->singleton( Black_Friday::class, Black_Friday::class, [ 'hook' ] );
Copy link
Contributor

Choose a reason for hiding this comment

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

[phpcs] reported by reviewdog 🐶
PEAR.Functions.FunctionCallSignature.SpaceAfterOpenBracket
Expected 1 spaces after opening parenthesis; 2 found

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
$this->container->singleton( Black_Friday::class, Black_Friday::class, [ 'hook' ] );
$this->container->singleton( Black_Friday::class, Black_Friday::class, [ 'hook' ] );

@@ -0,0 +1,182 @@
<?php
Copy link
Contributor

Choose a reason for hiding this comment

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

[phpcs] reported by reviewdog 🐶
Squiz.Commenting.FileComment.Missing
Missing file doc comment


return $content;
class End_Of_Year_Sale {
public function __call( $name, $arguments ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

[phpcs] reported by reviewdog 🐶
Squiz.Commenting.FunctionComment.Missing
Missing doc comment for function __call()

$date = $date->setTime( 23, 59 );

return $date;
public static function __callStatic( $name, $arguments ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

[phpcs] reported by reviewdog 🐶
Squiz.Commenting.FunctionComment.Missing
Missing doc comment for function __callStatic()

}
}
class Stellar_Sale {
public function __call( $name, $arguments ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

[phpcs] reported by reviewdog 🐶
Squiz.Commenting.FunctionComment.Missing
Missing doc comment for function __call()


return $this->get_template()->template( 'notices/tribe-stellar-sale', $template_args, false );
public static function __callStatic( $name, $arguments ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

[phpcs] reported by reviewdog 🐶
Squiz.Commenting.FunctionComment.Missing
Missing doc comment for function __callStatic()

}

/**
/**
Copy link
Contributor

Choose a reason for hiding this comment

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

[phpcs] reported by reviewdog 🐶
Generic.WhiteSpace.ScopeIndent.Incorrect
Line indented incorrectly; expected at least 1 tabs, found 0

* This method is used to enqueue additional assets for the admin notices.
* Each should conditionally call an internal `enqueue_additional_assets()` function to handle the enqueueing.
*
* @since 5.1.10
* @deprecated TBD
Copy link
Contributor

Choose a reason for hiding this comment

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

[phpcs] reported by reviewdog 🐶
Generic.WhiteSpace.DisallowSpaceIndent.SpacesUsed
Tabs must be used to indent lines; spaces are not allowed

* This method is used to enqueue additional assets for the admin notices.
* Each should conditionally call an internal `enqueue_additional_assets()` function to handle the enqueueing.
*
* @since 5.1.10
* @deprecated TBD
Copy link
Contributor

Choose a reason for hiding this comment

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

[phpcs] reported by reviewdog 🐶
Squiz.Commenting.DocCommentAlignment.SpaceBeforeStar
Expected 1 space before asterisk; 5 found

*/

$sale_year = date_i18n( 'Y' );
/* translators: %1$s: Black Friday year */
Copy link
Contributor

Choose a reason for hiding this comment

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

[phpcs] reported by reviewdog 🐶
Squiz.Commenting.BlockComment.NoNewLine
Block comment text must start on a new line

Camwyn
Camwyn previously approved these changes Oct 15, 2024
Camwyn
Camwyn previously approved these changes Oct 15, 2024

$button_attr = new Attributes(
[
'style' => 'position: absolute; top: 0; right: 0; background: transparent; border: 0; color: #fff; padding: 0.5em; cursor: pointer;',
Copy link
Contributor

Choose a reason for hiding this comment

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

[phpcs] reported by reviewdog 🐶
WordPress.Arrays.MultipleStatementAlignment.DoubleArrowNotAligned
Array double arrow not aligned correctly; expected 1 space(s) between "'style'" and double arrow, but found 39.


// Dismiss button attributes.
'data-tec-conditional-content-dismiss-button' => true,
'data-tec-conditional-content-dismiss-slug' => $this->slug,
Copy link
Contributor

Choose a reason for hiding this comment

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

[phpcs] reported by reviewdog 🐶
WordPress.Arrays.MultipleStatementAlignment.LongIndexSpaceBeforeDoubleArrow
Expected 1 space between "'data-tec-conditional-content-dismiss-slug'" and double arrow; 3 found.

// Dismiss button attributes.
'data-tec-conditional-content-dismiss-button' => true,
'data-tec-conditional-content-dismiss-slug' => $this->slug,
'data-tec-conditional-content-dismiss-nonce' => $this->get_nonce(),
Copy link
Contributor

Choose a reason for hiding this comment

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

[phpcs] reported by reviewdog 🐶
WordPress.Arrays.MultipleStatementAlignment.LongIndexSpaceBeforeDoubleArrow
Expected 1 space between "'data-tec-conditional-content-dismiss-nonce'" and double arrow; 2 found.

@bordoni bordoni merged commit 26a91dc into release/T24.ettin Oct 15, 2024
11 of 17 checks passed
@bordoni bordoni deleted the fix/marketing-notices branch October 15, 2024 20:35
Camwyn pushed a commit that referenced this pull request Oct 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code review Status: requires a code review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants