Skip to content
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
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
1 change: 1 addition & 0 deletions src/blocks/my-account-button/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ Provides a reader account/sign-in button for sites using Newspack Reader Activat
## Settings
- Signed in label (`signedInLabel`): Text shown when the reader is authenticated.
- Signed out label (`signedOutLabel`): Text shown when the reader is not authenticated.
- Display: ability to toggle on/off the display of the button's icon or text label. Note: you cannot toggle off both the icon and label at the same time.

## Editor behavior
- Use the toolbar toggle (Signed in / Signed out) to edit each label.
Expand Down
4 changes: 4 additions & 0 deletions src/blocks/my-account-button/block.json
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,10 @@
"signedOutLabel": {
"type": "string",
"default": "Sign in"
},
"className": {
"type": "string",
"default": ""
}
},
"supports": {
Expand Down
51 changes: 35 additions & 16 deletions src/blocks/my-account-button/class-my-account-button-block.php
Original file line number Diff line number Diff line change
Expand Up @@ -93,13 +93,26 @@ public static function render_block( $attrs ) {
$default_attrs = [
'signedInLabel' => __( 'My Account', 'newspack-plugin' ),
'signedOutLabel' => __( 'Sign in', 'newspack-plugin' ),
'className' => '',
];
$attrs = \wp_parse_args( $attrs, $default_attrs );

$is_signed_in = \is_user_logged_in();
$label = $is_signed_in ? $attrs['signedInLabel'] : $attrs['signedOutLabel'];
if ( '' === trim( (string) $label ) ) {
return '';
$is_signed_in = \is_user_logged_in();
$signed_in_label = '' === trim( (string) $attrs['signedInLabel'] ) ? $default_attrs['signedInLabel'] : $attrs['signedInLabel'];
$signed_out_label = '' === trim( (string) $attrs['signedOutLabel'] ) ? $default_attrs['signedOutLabel'] : $attrs['signedOutLabel'];
$label = $is_signed_in ? $signed_in_label : $signed_out_label;

/** Display mode from block style class in className (default = icon + text). */
Copy link
Member

Choose a reason for hiding this comment

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

Nit: WPCS recommends // for inline comments. /** */ style is reserved for docblocks.

Copy link
Contributor

Choose a reason for hiding this comment

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

$wrapper_class = (string) $attrs['className'];
if ( \strpos( $wrapper_class, 'is-style-icon-only' ) !== false ) {
Copy link
Member

Choose a reason for hiding this comment

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

A hypothetical additional class like is-style-icon-only-alt would also match. A more robust approach:

$classes = explode( ' ', (string) $attrs['className'] );
if ( in_array( 'is-style-icon-only', $classes, true ) ) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call! Fixed in a49f0e0.

$show_label = false;
$show_icon = true;
} elseif ( \strpos( $wrapper_class, 'is-style-text-only' ) !== false ) {
$show_label = true;
$show_icon = false;
} else {
$show_label = true;
$show_icon = true;
}

$account_url = self::get_account_url();
Expand All @@ -118,8 +131,8 @@ public static function render_block( $attrs ) {
}

$labels = [
'signedin' => $attrs['signedInLabel'],
'signedout' => $attrs['signedOutLabel'],
'signedin' => $signed_in_label,
'signedout' => $signed_out_label,
];

$extra_classes = [
Expand Down Expand Up @@ -153,20 +166,26 @@ public static function render_block( $attrs ) {
$wrapper_div_classes = \array_merge( $wrapper_div_classes, $custom_classes );
}

$wrapper_attributes = \get_block_wrapper_attributes(
[
'class' => implode( ' ', $extra_classes ),
'href' => \esc_url_raw( $href ),
]
);
$wrapper_attribute_args = [
'class' => implode( ' ', $extra_classes ),
'href' => \esc_url_raw( $href ),
'data-wp-logged-in' => $is_signed_in ? '1' : '0',
Copy link
Member

Choose a reason for hiding this comment

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

The data-wp-* prefix is used by WordPress Interactivity API, and custom plugins should avoid it to prevent future collisions. Can we rename it to something different, like data-newspack-logged-in?

Also, this attribute explicitly states the user's authentication state in the DOM, though it's already inferable from the button's href (hashtag vs anything else). Maybe this is not really needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @rbcorrales! I updated the attribute name in 81ee30d, but left it -- it's a little yucky, but the My Account Button block behaves a little different than the My Account link that's inserted into the theme (the latter isn't visible for logged in non-readers, and when it is in the Customizer, it says 'Sign In'). The attribute seemed like the nicest way to switch it in just the one spot.

];
$wrapper_attributes = \get_block_wrapper_attributes( $wrapper_attribute_args );

$link = '<div class="' . \esc_attr( implode( ' ', $wrapper_div_classes ) ) . '">';
$link .= '<div class="wp-block-button">';
$link .= '<a ' . $wrapper_attributes . ' data-labels="' . \esc_attr( \wp_json_encode( $labels ) ) . '" ' . $should_modal_trigger . '>';
$link .= '<span class="wp-block-newspack-my-account-button__icon" aria-hidden="true">';
$link .= Newspack_UI_Icons::get_svg( 'account' );
$link .= '</span>';
$link .= '<span class="newspack-reader__account-link__label">' . \esc_html( $label ) . '</span>';
if ( $show_icon ) {
$link .= '<span class="wp-block-newspack-my-account-button__icon" aria-hidden="true">';
$link .= Newspack_UI_Icons::get_svg( 'account' );
$link .= '</span>';
}
$label_classes = [ 'newspack-reader__account-link__label' ];
if ( ! $show_label ) {
$label_classes[] = 'screen-reader-text';
}
$link .= '<span class="' . \esc_attr( implode( ' ', $label_classes ) ) . '">' . \esc_html( $label ) . '</span>';
$link .= '</a>';
$link .= '</div>';
$link .= '</div>';
Expand Down
85 changes: 51 additions & 34 deletions src/blocks/my-account-button/edit.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,16 +24,23 @@ import {
} from '@wordpress/block-editor';
import { ToolbarButton, ToolbarGroup } from '@wordpress/components';

/**
* Internal dependencies
*/
function MyAccountButtonEdit( { attributes, setAttributes } ) {
const { signedInLabel, signedOutLabel, style, className: customClassName } = attributes;
function MyAccountButtonEdit( { attributes, setAttributes, className: wrapperClassName } ) {
const { signedInLabel, signedOutLabel, style, className: blockClassName } = attributes;
const borderProps = useBorderProps( attributes );
const colorProps = useColorProps( attributes );
const spacingProps = useSpacingProps( attributes );

// Block style comes from the Styles panel (wrapper) or saved className.
const blockWrapperProps = useBlockProps();
Copy link
Member

Choose a reason for hiding this comment

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

Calling useBlockProps() twice in the same component is not a supported pattern. Each call generates its own ref and block binding, so the editor will bind internal state (selection, focus, drag) to whichever ref was registered last. The first call here is never attached to a DOM node, which can cause subtle bugs.

The is-style-* class is already available on attributes.className (already serialized by WP). You can technically derive the style variant directly from blockClassName without this extra call:

const isIconOnly = ( blockClassName || '' ).includes( 'is-style-icon-only' );
const isTextOnly = ( blockClassName || '' ).includes( 'is-style-text-only' );

Copy link
Contributor

Choose a reason for hiding this comment

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

const className = blockWrapperProps?.className ?? wrapperClassName ?? blockClassName ?? '';
const isIconOnly = className.includes( 'is-style-icon-only' );
const isTextOnly = className.includes( 'is-style-text-only' );
Copy link
Member

Choose a reason for hiding this comment

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

Similar comment about class names matching.

Copy link
Contributor

Choose a reason for hiding this comment

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

const isLabelVisible = ! isIconOnly;
const isIconVisible = ! isTextOnly;

const blockProps = useBlockProps( {
className: classnames(
className,
'wp-block-button__link',
'newspack-reader__account-link',
'wp-block-newspack-my-account-button__link',
Expand All @@ -51,52 +58,62 @@ function MyAccountButtonEdit( { attributes, setAttributes } ) {
...spacingProps.style,
},
} );
const isReaderActivationEnabled = typeof newspack_blocks === 'undefined' || newspack_blocks.has_reader_activation;

const isReaderActivationEnabled = typeof newspack_blocks === 'undefined' || newspack_blocks.has_reader_activation;
const [ previewState, setPreviewState ] = useState( 'signedout' );
const isSignedOutPreview = previewState === 'signedout';
const activeLabel = isSignedOutPreview ? signedOutLabel : signedInLabel;
const placeholderText = isSignedOutPreview ? __( 'Sign in', 'newspack-plugin' ) : __( 'My Account', 'newspack-plugin' );

function setButtonText( newText ) {
const cleaned = stripHTML( newText );
setAttributes( isSignedOutPreview ? { signedOutLabel: cleaned } : { signedInLabel: cleaned } );
setAttributes( isSignedOutPreview ? { signedOutLabel: stripHTML( newText ) } : { signedInLabel: stripHTML( newText ) } );
}

if ( ! isReaderActivationEnabled ) {
return <div { ...blockProps } style={ { ...blockProps.style, display: 'none' } } />;
}

return ! isReaderActivationEnabled ? (
<div { ...blockProps } style={ { ...blockProps.style, display: 'none' } } />
) : (
return (
<>
<BlockControls>
<ToolbarGroup>
<ToolbarButton
isPressed={ isSignedOutPreview }
onClick={ () => setPreviewState( 'signedout' ) }
style={ { paddingLeft: '12px', paddingRight: '12px' } }
>
{ __( 'Signed out', 'newspack-plugin' ) }
</ToolbarButton>
<ToolbarButton
isPressed={ ! isSignedOutPreview }
onClick={ () => setPreviewState( 'signedin' ) }
style={ { paddingLeft: '12px', paddingRight: '12px' } }
>
{ __( 'Signed in', 'newspack-plugin' ) }
</ToolbarButton>
</ToolbarGroup>
</BlockControls>
<div className={ classnames( 'wp-block-buttons', customClassName ) }>
{ isLabelVisible && (
Copy link
Member

Choose a reason for hiding this comment

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

When icon-only style is selected, isLabelVisible is false, so the "Signed out"/"Signed in" toolbar toggle disappears entirely. The user cannot edit the screen reader label text for the signed-in state without switching styles first, and then back.
I'm not sure if there's a solution for this, since it needs to render the text in order to be modified, or if this is even an issue 🤔.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah but I think it's OK since it uses the default aria-labels

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is a little funky, but I think it's probably okay as is for now -- or at least the kind of thing that should be covered in the documentation (how to edit the hidden label), and if we get a lot of feedback about friction, we can revisit 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Readme has been updated in cf68793!

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 say keep it as it is for now. If we get feedback about this, we can always add 2 textcontrols to the sidebar

<BlockControls>
<ToolbarGroup>
<ToolbarButton
icon={ false }
isPressed={ isSignedOutPreview }
label={ __( 'Signed out', 'newspack-plugin' ) }
onClick={ () => setPreviewState( 'signedout' ) }
style={ { paddingLeft: '12px', paddingRight: '12px' } }
>
{ __( 'Signed out', 'newspack-plugin' ) }
</ToolbarButton>
<ToolbarButton
icon={ false }
isPressed={ ! isSignedOutPreview }
label={ __( 'Signed in', 'newspack-plugin' ) }
onClick={ () => setPreviewState( 'signedin' ) }
style={ { paddingLeft: '12px', paddingRight: '12px' } }
>
{ __( 'Signed in', 'newspack-plugin' ) }
</ToolbarButton>
</ToolbarGroup>
</BlockControls>
) }
<div className={ classnames( 'wp-block-buttons', blockClassName ) }>
<div className="wp-block-button">
<div { ...blockProps }>
<span className="wp-block-newspack-my-account-button__icon" aria-hidden="true">
{ icon }
</span>
{ isIconVisible && (
<span className="wp-block-newspack-my-account-button__icon" aria-hidden="true">
{ icon }
</span>
) }
<RichText
tagName="span"
className={ ! isLabelVisible ? 'screen-reader-text' : undefined }
aria-label={ __( 'Button text', 'newspack-plugin' ) }
placeholder={ placeholderText }
value={ activeLabel || '' }
onChange={ value => setButtonText( value ) }
onChange={ setButtonText }
withoutInteractiveFormatting
/>
</div>
Expand Down
18 changes: 18 additions & 0 deletions src/blocks/my-account-button/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,30 @@ import './style.scss';
/**
* WordPress dependencies
*/
import { registerBlockStyle } from '@wordpress/blocks';
import { __ } from '@wordpress/i18n';
import { button as icon } from '@wordpress/icons';

const { name } = metadata;

export { metadata, name };

registerBlockStyle( name, {
name: 'default',
label: __( 'Default', 'newspack-plugin' ),
isDefault: true,
} );

registerBlockStyle( name, {
name: 'icon-only',
label: __( 'Icon only', 'newspack-plugin' ),
} );

registerBlockStyle( name, {
name: 'text-only',
label: __( 'Text only', 'newspack-plugin' ),
} );

export const settings = {
title: metadata.title,
icon: {
Expand Down
11 changes: 8 additions & 3 deletions src/blocks/my-account-button/style.scss
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,18 @@
&__icon {
display: grid;
place-items: center;
}

svg {
fill: currentcolor;
height: max(24px, 1.5em);
width: max(24px, 1.5em);
}

&:not(:has(.screen-reader-text)) {
svg {
fill: currentcolor;
height: max(24px, 1.5em);
margin-left: calc( max(4px, 0.25em) * -1);
margin-right: calc( max(4px, 0.25em) * -1);
width: max(24px, 1.5em);
}
}
}
5 changes: 5 additions & 0 deletions src/reader-activation-auth/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,11 @@ window.newspackRAS.push( readerActivation => {
const labels = JSON.parse( link.getAttribute( 'data-labels' ) );
const labelEl = link.querySelector( '.newspack-reader__account-link__label' );
if ( labelEl ) {
const isLoggedIn = link.getAttribute( 'data-wp-logged-in' ) === '1';
if ( isLoggedIn ) {
labelEl.textContent = labels.signedin;
return;
}
labelEl.textContent = reader?.authenticated ? labels.signedin : labels.signedout;

// Set my account link href if the reader is authenticated.
Expand Down
74 changes: 70 additions & 4 deletions tests/unit-tests/class-test-my-account-button-block.php
Original file line number Diff line number Diff line change
Expand Up @@ -96,16 +96,81 @@ public function test_render_block_signed_in() {
}

/**
* Test empty labels -- render nothing if a button's state has an empty label.
* Test empty signed-out label falls back to default.
*/
public function test_render_block_empty_label() {
public function test_render_block_empty_signed_out_label() {
self::$reader_activation_enabled = true;

$signed_out_output = do_blocks(
'<!-- wp:newspack/my-account-button {"signedInLabel":"My Account","signedOutLabel":""} /-->'
);

$this->assertSame( '', trim( $signed_out_output ) );
$this->assertNotEmpty( $signed_out_output );
$this->assertStringContainsString( '&quot;signedout&quot;:&quot;Sign in&quot;', $signed_out_output );
}

/**
* Test icon-only style applies the screen-reader-text class to the label.
*/
public function test_render_block_icon_only_style_adds_screen_reader_text_class() {
self::$reader_activation_enabled = true;

$output = do_blocks(
'<!-- wp:newspack/my-account-button {"signedInLabel":"My Account","signedOutLabel":"Sign in","className":"is-style-icon-only"} /-->'
);

$this->assertNotEmpty( $output );
$this->assertStringContainsString( 'newspack-reader__account-link__label screen-reader-text', $output );
}

/**
* Test text-only style removes the icon markup.
*/
public function test_render_block_text_only_style_removes_icon_markup() {
self::$reader_activation_enabled = true;

$output = do_blocks(
'<!-- wp:newspack/my-account-button {"signedInLabel":"My Account","signedOutLabel":"Sign in","className":"is-style-text-only"} /-->'
);

$this->assertNotEmpty( $output );
$this->assertStringNotContainsString( 'wp-block-newspack-my-account-button__icon', $output );
}

/**
* Test default style renders the icon markup.
*/
public function test_render_block_default_style_renders_icon_markup() {
self::$reader_activation_enabled = true;

$output = do_blocks(
'<!-- wp:newspack/my-account-button {"signedInLabel":"My Account","signedOutLabel":"Sign in"} /-->'
);

$this->assertNotEmpty( $output );
$this->assertStringContainsString( 'wp-block-newspack-my-account-button__icon', $output );
}

/**
* Test default style (no className) renders both label and icon.
*/
public function test_render_block_default_style_renders_label_and_icon() {
self::$reader_activation_enabled = true;

$output = do_blocks(
'<!-- wp:newspack/my-account-button {"signedInLabel":"My Account","signedOutLabel":"Sign in"} /-->'
);

$this->assertNotEmpty( $output );
$this->assertStringContainsString( 'newspack-reader__account-link__label', $output );
$this->assertStringContainsString( 'wp-block-newspack-my-account-button__icon', $output );
}

/**
* Test empty signed-in label falls back to default.
*/
public function test_render_block_empty_signed_in_label() {
self::$reader_activation_enabled = true;

$user_id = self::factory()->user->create(
[
Expand All @@ -118,7 +183,8 @@ public function test_render_block_empty_label() {
'<!-- wp:newspack/my-account-button {"signedInLabel":"","signedOutLabel":"Sign in"} /-->'
);

$this->assertSame( '', trim( $signed_in_output ) );
$this->assertNotEmpty( $signed_in_output );
$this->assertStringContainsString( '&quot;signedin&quot;:&quot;My Account&quot;', $signed_in_output );
}

/**
Expand Down
1 change: 1 addition & 0 deletions webpack.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ const entry = {
'content-gate-countdown-box-block': path.join( __dirname, 'src', 'blocks', 'content-gate', 'countdown-box', 'index.js' ),
'contribution-meter-block': path.join( __dirname, 'src', 'blocks', 'contribution-meter', 'index.js' ),
'avatar-block': path.join( __dirname, 'src', 'blocks', 'avatar', 'index.js' ),
'my-account-button-block': path.join( __dirname, 'src', 'blocks', 'my-account-button', 'index.js' ),
'my-account': path.join( __dirname, 'src', 'my-account', 'index.js' ),
'my-account-v0': path.join( __dirname, 'src', 'my-account', 'v0', 'index.js' ),
'my-account-v1': path.join( __dirname, 'src', 'my-account', 'v1', 'index.js' ),
Expand Down
Loading