Show notice every 4 weeks #43
Conversation
src/Whip_WPDismissOption.php
Outdated
| } | ||
|
|
||
| /** | ||
| * Returns the value of the whip_dismissed option. | ||
| * | ||
| * @return string Returns the value of the option or an empty string when not set. | ||
| * @return string|int Returns the value of the option or an empty string when not set. |
There was a problem hiding this comment.
As we are saving a timestamp, we should just return an int and cast the stored value to int when returning it.
src/Whip_MessageDismisser.php
Outdated
| $parts = explode( '.', $versionToConvert, 3 ); | ||
|
|
||
| return implode( '.', array_slice( $parts, 0, 2 ) ); | ||
| return ( ( $this->storage->get() + $this->threshold ) > $this->currentTime ); |
There was a problem hiding this comment.
I find this pretty hard to read.
Prefer to make it better readable by flipping the variables around a bit:
( $this->currentTime <= ( $this->storage->get() + $this->threshold ) )
The description of the method should follow the logic.
Checks if the current time is lower than the stored time extended by the threshold.
src/Whip_WPDismissOption.php
Outdated
| @@ -11,23 +11,23 @@ class Whip_WPDismissOption implements Whip_DismissStorage { | |||
| /** | |||
| * Saves the value to the options. | |||
| * | |||
| * @param string $dismissedVersion The value to save. | |||
| * @param string $dismissedValue The value to save. | |||
There was a problem hiding this comment.
As we are saving a timestamp, this can be an int
| @@ -48,9 +48,10 @@ public function renderMessage() { | |||
|
|
|||
| /* translators: 1: is a link to dismiss url 2: closing link tag */ | |||
| $dismissButton = sprintf( | |||
| __( '<p>%1$sRemind me again after the next WordPress release.%2$s</p>', 'wordpress' ), | |||
| __( '<p>%1$sRemind me again after %3$d weeks.%2$s</p>', 'wordpress' ), | |||
There was a problem hiding this comment.
The number can be hard-coded, as it will not change.
| @@ -48,7 +48,7 @@ public function renderMessage() { | |||
|
|
|||
| /* translators: 1: is a link to dismiss url 2: closing link tag */ | |||
| $dismissButton = sprintf( | |||
| __( '<p>%1$sRemind me again after the next WordPress release.%2$s</p>', 'wordpress' ), | |||
| __( '<p>%1$sRemind me again after 4 weeks.%2$s</p>', 'wordpress' ), | |||
There was a problem hiding this comment.
Please remove the <p> tags from the translation.
There was a problem hiding this comment.
Please change after to in
Change the `after` to an `in`
The presenter should not know of any threshold period, which was entered in the message.
|
CR & Acceptance done 👍 |
Instead of checking on major WP Version change we are now showing the notice every 4 weeks.
Test instructions
Fixes #42