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

feat(reader-revenue): make NYP and Stripe Gateway optional #1645

Merged
merged 18 commits into from
Feb 6, 2024
Merged

feat(reader-revenue): make NYP and Stripe Gateway optional #1645

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
79c2612
fix(donate): don’t require Name Your Price extension
dkoo Jan 17, 2024
db8797e
feat(donate): force untiered and non-manual when NYP is available
dkoo Jan 19, 2024
c7c5b5c
fix: typescript lint errors
dkoo Jan 19, 2024
bb3b2e1
Merge branch 'master' into fix/optional-nyp-stripe-gateway
dkoo Jan 24, 2024
9f528b8
Update src/blocks/donate/frontend/class-newspack-blocks-donate-render…
dkoo Jan 25, 2024
a991eb8
Merge branch 'master' into fix/optional-nyp-stripe-gateway
dkoo Jan 26, 2024
a985f00
fix: undefined array key warning
dkoo Jan 26, 2024
308181d
fix: hide layout options if NYP not available
dkoo Jan 26, 2024
3701068
fix: make amount + frequency label translatable
dkoo Jan 30, 2024
42e145b
chore: remove unused variables
dkoo Jan 30, 2024
3a15ae5
fix: eslint errors
dkoo Jan 30, 2024
5f9479a
refactor: use wc and wcs methods to get formatted price
dkoo Jan 31, 2024
977db4b
fix: add missing TS type
dkoo Jan 31, 2024
2b65371
fix: add missing TS type
dkoo Feb 1, 2024
c388896
Merge branch 'trunk' into fix/optional-nyp-stripe-gateway
dkoo Feb 5, 2024
5b8470c
fix: avoid NaN with empty number inputs
dkoo Feb 5, 2024
9f041db
fix: show minimum suggestion warning for null values
dkoo Feb 6, 2024
8c01e69
Merge branch 'trunk' into fix/optional-nyp-stripe-gateway
dkoo Feb 6, 2024
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
23 changes: 10 additions & 13 deletions includes/class-modal-checkout.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,20 +219,17 @@ function ( $item ) {
}

/** Apply NYP custom price */
if ( class_exists( 'WC_Name_Your_Price_Helpers' ) ) {
$is_product_nyp = \WC_Name_Your_Price_Helpers::is_nyp( $product_id );
$price = filter_input( INPUT_GET, 'price', FILTER_SANITIZE_FULL_SPECIAL_CHARS );
if ( $is_product_nyp ) {
if ( empty( $price ) ) {
$price = \WC_Name_Your_Price_Helpers::get_suggested_price( $product_id );
}
$min_price = \WC_Name_Your_Price_Helpers::get_minimum_price( $product_id );
$max_price = \WC_Name_Your_Price_Helpers::get_maximum_price( $product_id );
$price = ! empty( $max_price ) ? min( $price, $max_price ) : $price;
$price = ! empty( $min_price ) ? max( $price, $min_price ) : $price;

$cart_item_data['nyp'] = (float) \WC_Name_Your_Price_Helpers::standardize_number( $price );
$price = filter_input( INPUT_GET, 'price', FILTER_SANITIZE_FULL_SPECIAL_CHARS );
if ( \Newspack_Blocks::can_use_name_your_price() ? \WC_Name_Your_Price_Helpers::is_nyp( $product_id ) : false ) {
if ( empty( $price ) ) {
$price = \WC_Name_Your_Price_Helpers::get_suggested_price( $product_id );
}
$min_price = \WC_Name_Your_Price_Helpers::get_minimum_price( $product_id );
$max_price = \WC_Name_Your_Price_Helpers::get_maximum_price( $product_id );
$price = ! empty( $max_price ) ? min( $price, $max_price ) : $price;
$price = ! empty( $min_price ) ? max( $price, $min_price ) : $price;

$cart_item_data['nyp'] = (float) \WC_Name_Your_Price_Helpers::standardize_number( $price );
}

/**
Expand Down
10 changes: 10 additions & 0 deletions includes/class-newspack-blocks.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,6 +230,15 @@ function( $tax ) {
return apply_filters( 'newspack_blocks_home_page_block_custom_taxonomies', $custom_taxonomies );
}

/**
* Check if the Name Your Price extension is available.
*
* @return bool True if available, false if not.
*/
public static function can_use_name_your_price() {
return class_exists( 'WC_Name_Your_Price_Helpers' );
}

/**
* Enqueue block scripts and styles for editor.
*/
Expand Down Expand Up @@ -257,6 +266,7 @@ public static function enqueue_block_editor_assets() {
'has_recaptcha' => class_exists( 'Newspack\Recaptcha' ) && \Newspack\Recaptcha::can_use_captcha(),
'recaptcha_url' => admin_url( 'admin.php?page=newspack-connections-wizard' ),
'custom_taxonomies' => self::get_custom_taxonomies(),
'can_use_name_your_price' => self::can_use_name_your_price(),
];

if ( class_exists( 'WP_REST_Newspack_Author_List_Controller' ) ) {
Expand Down
6 changes: 5 additions & 1 deletion src/blocks/checkout-button/edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
/* eslint-disable @wordpress/no-unsafe-wp-apis */
/* globals newspack_blocks_data */

/**
* External dependencies
*/
Expand Down Expand Up @@ -47,7 +49,9 @@ function getVariationName( variation ) {

function getNYP( product ) {
return {
isNYP: product?.meta_data?.some( meta => meta.key === '_nyp' && meta.value === 'yes' ),
isNYP:
newspack_blocks_data?.can_use_name_your_price &&
product?.meta_data?.some( meta => meta.key === '_nyp' && meta.value === 'yes' ),
suggestedPrice: product?.meta_data?.find( meta => meta.key === '_suggested_price' )?.value,
minPrice: product?.meta_data?.find( meta => meta.key === '_min_price' )?.value,
maxPrice: product?.meta_data?.find( meta => meta.key === '_maximum_price' )?.value,
Expand Down
82 changes: 57 additions & 25 deletions src/blocks/donate/edit/FrequencyBasedLayout.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import { RichText } from '@wordpress/block-editor';
* Internal dependencies
*/
import { AmountValueInput } from './components';
import { getColorForContrast } from '../utils';
import { getColorForContrast, getFrequencyLabel } from '../utils';
import { FREQUENCIES } from '../consts';
import type { ComponentProps, DonationFrequencySlug } from '../types';

Expand Down Expand Up @@ -55,7 +55,7 @@ const FrequencyBasedLayout = ( props: { isTiered: boolean } & ComponentProps ) =
}, [ attributes.defaultFrequency ] );

const [ selectedFrequency, setSelectedFrequency ] = useState( attributes.defaultFrequency );

const canUseNameYourPrice = window.newspack_blocks_data?.can_use_name_your_price;
const renderFrequencySelect = ( frequencySlug: DonationFrequencySlug ) => (
<>
<input
Expand Down Expand Up @@ -96,31 +96,63 @@ const FrequencyBasedLayout = ( props: { isTiered: boolean } & ComponentProps ) =
<div className="wp-block-newspack-blocks-donate__options">
<div className="wp-block-newspack-blocks-donate__frequencies frequencies">
<div className="tab-container">{ availableFrequencies.map( renderTab ) }</div>
{ availableFrequencies.map( frequencySlug => (
<div
className="wp-block-newspack-blocks-donate__frequency frequency"
key={ frequencySlug }
>
{ renderFrequencySelect( frequencySlug ) }
<div className="input-container">
<label
className="donate-label"
htmlFor={ 'newspack-' + frequencySlug + '-' + uid + '-untiered-input' }
>
{ __( 'Donation amount', 'newspack-blocks' ) }
</label>
<div className="wp-block-newspack-blocks-donate__money-input money-input">
<span className="currency">{ settings.currencySymbol }</span>
<AmountValueInput
{ ...props }
frequencySlug={ frequencySlug }
tierIndex={ 3 }
id={ `newspack-${ frequencySlug }-${ uid }-untiered-input` }
/>
{ availableFrequencies.map( frequencySlug => {
const untieredAmount = amounts[ frequencySlug ][ 3 ];
return (
<div
className="wp-block-newspack-blocks-donate__frequency frequency"
key={ frequencySlug }
>
{ renderFrequencySelect( frequencySlug ) }
<div className="input-container">
{ canUseNameYourPrice ? (
<>
<label
className="donate-label"
htmlFor={ 'newspack-' + frequencySlug + '-' + uid + '-untiered-input' }
>
{ __( 'Donation amount', 'newspack-blocks' ) }
</label>
<div className="wp-block-newspack-blocks-donate__money-input money-input">
<span className="currency">{ settings.currencySymbol }</span>
<AmountValueInput
{ ...props }
frequencySlug={ frequencySlug }
tierIndex={ 3 }
id={ `newspack-${ frequencySlug }-${ uid }-untiered-input` }
/>
</div>
</>
) : (
<>
<input
type="radio"
value={ untieredAmount }
className={ 'frequency-input' }
id={ `newspack-${ frequencySlug }-${ uid }-untiered-input` }
name={ `donation_value_${ frequencySlug }` }
defaultChecked={ true }
/>
<label
className="tier-select-label tier-label"
htmlFor={ `newspack-${ frequencySlug }-${ uid }-untiered-input` }
>
<div
dangerouslySetInnerHTML={ {
__html: getFrequencyLabel(
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we can have this string passed from the backend via newspack_blocks_data since the logic for creating the string already lives there? Then we can remove the duplicate logic from the frontend.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 5f9479a. Because the block needs to be able to dynamically render values from attributes, this may look a little janky with the string replacement, but I'm open to better suggestions.

Copy link
Contributor

@chickenn00dle chickenn00dle Feb 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Ah. I see what you mean. It definitely does seem a bit overly complex. I'm not sure of a cleaner way to handle this with the existing design for the donate block. My suggestion would be to remove the per X/once from the amount strings altogether as this is implied based on the category tab selection at the top (one-time/monthly/annually). But I think changing the design of this might be outside of the scope of this PR.

I think what we have here is fine, although there is an edge case where the publisher has an empty value set, for example while editing an amount. The block shows NaN:

Screenshot 2024-02-02 at 10 24 48

Copy link
Contributor Author

Choose a reason for hiding this comment

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

5b8470c forces the value to be 0 in this case. Maybe still not ideal, but better than $NaN?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah. I think it makes sense to set this to 0 when the input is empty. One other issue I noticed just now while retesting this is if the input is empty, we need to make sure the below $X validation still functions and prevents us from saving.

Here it is with an input value less than 5 for me:
Screenshot 2024-02-06 at 11 37 44

And while the input is empty:
Screenshot 2024-02-06 at 11 37 51

I think instead of forcing how the null value is rendered in the preview, we can force null values to be stored as 0 somewhere around here:

newspack-blocks/src/blocks/donate/edit/index.tsx

Line 109 in 5b8470c

setAttributes( { amounts: { ...donationSettings.amounts, ...attributes.amounts } } );

Copy link
Contributor

Choose a reason for hiding this comment

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

I also want to note while I was retesting this, I did some more thinking about the complexity of generating the translation string with dynamic pricing and HTML elements, and realized we can probably reduce the complexity by removing the span wrappers from the currency symbol and price, and instead placing them only around the frequency string.

We can achieve the same style results via CSS this way by setting the .wpbnbd__tiers__amount__value text to have the bolded and enlarged size and set the frequency span to have the unbolded smaller size.

This is how it is rendered right now:

<span class="wpbnbd__tiers__amount__value">
    <span class="woocommerce-Price-amount amount">
        <bdi>
            <span class="woocommerce-Price-currencySymbol">$</span>5
        </bdi>
    </span>
    once
</span>

We can probably achieve the same result with this:

<span class="wpbnbd__tiers__amount__value">
    $5
    <span class="some-class-for-frequency-label">
        once
    </span>
</span>

I'm okay if we tackle this another time though since this PR has become quite involved.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also want to note while I was retesting this, I did some more thinking about the complexity of generating the translation string with dynamic pricing and HTML elements, and realized we can probably reduce the complexity by removing the span wrappers from the currency symbol and price, and instead placing them only around the frequency string.

That's a good call, but yes, let's leave for another PR.

I think instead of forcing how the null value is rendered in the preview, we can force null values to be stored as 0 somewhere around here:

At first I tried forcing empty input values to be stored as 0, but this results in the input itself showing 0 when you delete the values in the input, which is potentially confusing to the user. Agree that we should still validate against the minimum donation value, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree that we should still validate against the minimum donation value, though.

Implemented in 9f041db

settings.currencySymbol,
untieredAmount.toFixed( 2 ).replace( /\.?0*$/, '' ),
frequencySlug
),
} }
/>
</label>
</>
) }
</div>
</div>
</div>
) ) }
);
} ) }
</div>
</div>
);
Expand Down
37 changes: 11 additions & 26 deletions src/blocks/donate/edit/TierBasedLayout.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,26 +9,10 @@ import classNames from 'classnames';
/**
* Internal dependencies
*/
import type {
ComponentProps,
DonationFrequencySlug,
DonateBlockAttributes,
TierBasedOptionValue,
} from '../types';
import { getColorForContrast } from '../utils';
import type { ComponentProps, DonateBlockAttributes, TierBasedOptionValue } from '../types';
import { getColorForContrast, getFrequencyLabel } from '../utils';
import { FREQUENCIES, DISABLED_IN_TIERS_BASED_LAYOUT_TIER_INDEX } from '../consts';

const getFrequencyLabel = ( frequencySlug: DonationFrequencySlug ) => {
switch ( frequencySlug ) {
case 'once':
return __( 'once', 'newspack-blocks' );
case 'month':
return __( 'per month', 'newspack-blocks' );
case 'year':
return __( 'per year', 'newspack-blocks' );
}
};

const TierBasedLayout = ( props: ComponentProps ) => {
const { amounts, availableFrequencies, attributes } = props;
const [ currentFrequency, setCurrencyFrequency ] = useState( availableFrequencies[ 0 ] );
Expand Down Expand Up @@ -108,14 +92,15 @@ const TierBasedLayout = ( props: ComponentProps ) => {
</h3>
</div>
<div className="wpbnbd__tiers__amount">
<h3 className="wpbnbd__tiers__amount__number">
{ props.settings.currencySymbol }
<span>{ amount }</span>
</h3>
<span className="wpbnbd__tiers__amount__frequency">
{ ' ' }
{ getFrequencyLabel( currentFrequency ) }
</span>
<div
dangerouslySetInnerHTML={ {
__html: getFrequencyLabel(
props.settings.currencySymbol,
amount.toFixed( 2 ).replace( /\.?0*$/, '' ),
currentFrequency
),
} }
/>
</div>
<div
className="submit-button"
Expand Down
Loading