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

Improve tag name determination #1989

Merged
merged 1 commit into from
Mar 18, 2019
Merged
Show file tree
Hide file tree
Changes from all 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
30 changes: 29 additions & 1 deletion assets/src/amp-story-editor-blocks.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ import {
Shortcuts,
} from './components';
import { ALLOWED_BLOCKS, ALLOWED_CHILD_BLOCKS } from './constants';
import { maybeEnqueueFontStyle, setBlockParent, addAMPAttributes, addAMPExtraProps } from './helpers';
import { maybeEnqueueFontStyle, setBlockParent, addAMPAttributes, addAMPExtraProps, getTagName } from './helpers';
import store from './stores/amp-story';

const { getBlockOrder, getBlock, getBlocksByClientId, getClientIdsWithDescendants, getBlockRootClientId } = select( 'core/editor' );
Expand Down Expand Up @@ -187,6 +187,33 @@ function maybeSetInitialPositioning( clientId ) {
}
}

/**
* Determines the HTML tag name that should be used for text blocks.
*
* This is based on the block's attributes, as well as the surrounding context.
*
* For example, there can only be one <h1> tag on a page.
* Also, font size takes precedence over text length as it's a stronger signal for semantic meaning.
*
* @param {string} clientId Block ID.
*/
function maybeSetTagName( clientId ) {
const block = getBlock( clientId );

if ( ! block || 'amp/amp-story-text' !== block.name ) {
return;
}

const siblings = getBlocksByClientId( getBlockOrder( clientId ) ).filter( ( { clientId: blockId } ) => blockId !== clientId );
const canUseH1 = ! siblings.some( ( { attributes } ) => attributes.tagName === 'h1' );

const tagName = getTagName( block.attributes, canUseH1 );

if ( block.attributes.tagName !== tagName ) {
updateBlockAttributes( clientId, { tagName } );
}
}

let blockOrder = getBlockOrder();
let allBlocksWithChildren = getClientIdsWithDescendants();

Expand Down Expand Up @@ -232,6 +259,7 @@ subscribe( () => {

for ( const block of allBlocksWithChildren ) {
maybeSetInitialPositioning( block );
maybeSetTagName( block );
}

allBlocksWithChildren = getClientIdsWithDescendants();
Expand Down
54 changes: 0 additions & 54 deletions assets/src/blocks/amp-story-text/get-tag-name.js

This file was deleted.

8 changes: 5 additions & 3 deletions assets/src/blocks/amp-story-text/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ import { registerBlockType } from '@wordpress/blocks';
* Internal dependencies
*/
import edit from './edit';
import getTagName from './get-tag-name';

export const name = 'amp/amp-story-text';

Expand All @@ -41,6 +40,10 @@ const schema = {
type: 'string',
default: 'auto',
},
tagName: {
type: 'string',
default: 'p',
},
fontSize: {
type: 'string',
},
Expand Down Expand Up @@ -94,14 +97,13 @@ export const settings = {
textColor,
customBackgroundColor,
customTextColor,
tagName,
} = attributes;

const textClass = getColorClassName( 'color', textColor );
const backgroundClass = getColorClassName( 'background-color', backgroundColor );
const fontSizeClass = getFontSizeClass( fontSize );

const tagName = getTagName( attributes );

const className = classnames( {
'has-text-color': textColor || customTextColor,
'has-background': backgroundColor || customBackgroundColor,
Expand Down
58 changes: 58 additions & 0 deletions assets/src/helpers.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,12 @@
*/
import uuid from 'uuid/v4';

/**
* WordPress dependencies
*/
import { count } from '@wordpress/wordcount';
import { _x } from '@wordpress/i18n';

/**
* Internal dependencies
*/
Expand Down Expand Up @@ -217,3 +223,55 @@ export const addAMPExtraProps = ( props, blockType, attributes ) => {
...ampAttributes,
};
};

// Todo: Make these customizable?
const H1_FONT_SIZE = 40;
const H2_FONT_SIZE = 24;
const H1_TEXT_LENGTH = 4;
const H2_TEXT_LENGTH = 10;

/*
* translators: If your word count is based on single characters (e.g. East Asian characters),
* enter 'characters_excluding_spaces' or 'characters_including_spaces'. Otherwise, enter 'words'.
* Do not translate into your own language.
*/
const wordCountType = _x( 'words', 'Word count type. Do not translate!', 'amp' );

/**
* Determines the HTML tag name that should be used given on the block's attributes.
*
* Font size takes precedence over text length as it's a stronger signal for semantic meaning.
*
* @param {Object} attributes Block attributes.
* @param {boolean} canUseH1 Whether an H1 tag is allowed.
*
* @return {string} HTML tag name. Either p, h1, or h2.
*/
export const getTagName = ( attributes, canUseH1 ) => {
const { fontSize, customFontSize, positionTop } = attributes;

// Elements positioned that low on a page are unlikely to be headings.
if ( positionTop > 80 ) {
return 'p';
}

if ( 'huge' === fontSize || ( customFontSize && customFontSize > H1_FONT_SIZE ) ) {
return canUseH1 ? 'h1' : 'h2';
}

if ( 'large' === fontSize || ( customFontSize && customFontSize > H2_FONT_SIZE ) ) {
return 'h2';
}

const textLength = count( attributes.content, wordCountType, {} );

if ( H1_TEXT_LENGTH >= textLength ) {
return canUseH1 ? 'h1' : 'h2';
}

if ( H2_TEXT_LENGTH >= textLength ) {
return 'h2';
}

return 'p';
};