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

Text: add drop cap support #1189

Merged
merged 6 commits into from
Jun 15, 2017
Merged

Text: add drop cap support #1189

merged 6 commits into from
Jun 15, 2017

Conversation

mtias
Copy link
Member

@mtias mtias commented Jun 15, 2017

@mtias mtias added [Feature] Blocks Overall functionality of blocks Design [Type] Enhancement A suggestion for improvement. labels Jun 15, 2017
@@ -56,6 +56,19 @@
}
}

.editor-visual-editor__block:not( .is-selected ) .blocks-editable.drop-cap-true {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we move this style into the text block stylesheet instead? (or create one)

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, I was expecting the style to trigger as soon as I toggle the inspector control. Why is it only for not selected blocks?

Copy link
Member Author

Choose a reason for hiding this comment

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

contentEditable cannot handle styling :first-letter, also when editing it is nicer to be able to see the whole line.

Agreed on moving to styles. Having some conversations on how to go about front-end styles in general.

@@ -44,6 +46,11 @@ registerBlockType( 'core/text', {
/>
</BlockControls>
),
focus && (
<InspectorControls>
<Toggle checked={ !! dropCap } onChange={ toggleDropCap } />
Copy link
Contributor

Choose a reason for hiding this comment

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

We probably need a label, Should we create an inspector ToggleControl (same as range and text)?

Copy link
Contributor

@paulwilde paulwilde Jun 15, 2017

Choose a reason for hiding this comment

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

@youknowriad I'll create one and submit a pull request.

@aduth
Copy link
Member

aduth commented Jun 15, 2017

The float can cause some funny effects to the layout, particularly in short text blocks:

Float

(Note Inserter misalignment)

Might need a clear on the block.

const { align, content } = attributes;

const { align, content, dropCap } = attributes;
const toggleDropCap = () => setAttributes( { dropCap: ! dropCap } );
return [
focus && (
Copy link
Member

@aduth aduth Jun 15, 2017

Choose a reason for hiding this comment

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

Possible alternative to creating single combined focus condition:

focus && ...[
	<BlockControls key="controls">
		<AlignmentToolbar
			value={ align }
			onChange={ ( nextAlign ) => {
				setAttributes( { align: nextAlign } );
			} }
		/>
	</BlockControls>,
	<InspectorControls key="inspector">
		<div className="blocks-text__drop-cap" style={ { display: 'flex', justifyContent: 'space-between' } }>
			<label htmlFor="blocks-text__drop-cap">{ __( 'Drop Cap' ) }</label>
			<Toggle
				checked={ !! dropCap }
				onChange={ toggleDropCap }
				id="blocks-text__drop-cap-toggle"
			/>
		</div>
	</InspectorControls>
],

Copy link
Member Author

Choose a reason for hiding this comment

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

This is something we'll need to pick a model and apply consistently, because people will start using as examples. I personally don't like the arrays much and prefer a single element tree.

Copy link
Member

Choose a reason for hiding this comment

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

In fairness, it's already an array 😄

@@ -10,11 +11,13 @@ import { registerBlockType, createBlock, query, setDefaultBlock } from '../../ap
import AlignmentToolbar from '../../alignment-toolbar';
import BlockControls from '../../block-controls';
import Editable from '../../editable';
import InspectorControls from '../../inspector-controls';
import Toggle from 'components/form-toggle';
Copy link
Member

Choose a reason for hiding this comment

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

This belongs in "WordPress dependencies"

@mtias mtias merged commit c521550 into master Jun 15, 2017
@mtias mtias deleted the add/drop-cap-support branch June 15, 2017 15:26
@@ -62,6 +77,7 @@ registerBlockType( 'core/text', {
} }
onMerge={ mergeBlocks }
style={ { textAlign: align } }
className={ `drop-cap-${ dropCap }` }
Copy link
Member

Choose a reason for hiding this comment

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

Should this be a modifier class on Editable?

className={ classnames( { 'has-drop-cap': dropCap } ) }
.blocks-editable.has-drop-cap .blocks-editable__tinymce:not( :focus ) {

Or better yet, a prop on the component if it's Editable's own styles responsible for creating the effect?

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed, plan is to turn it into has-drop-cap. I am not sure if it belong in editable, it feels like purely a text/paragraph addition. (Makes no sense in quotes, captions, etc.)

@@ -44,6 +47,18 @@ registerBlockType( 'core/text', {
/>
</BlockControls>
),
focus && (
<InspectorControls key="inspector">
<div className="blocks-text__drop-cap" style={ { display: 'flex', justifyContent: 'space-between' } }>
Copy link
Member

Choose a reason for hiding this comment

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

Why do we give this a className but then apply its styles inline? Should we move the styles to styles.scss?

I think elsewhere @youknowriad had mentioned creating better patterns for inspector sections with labels. Doesn't seem like it should be the responsibility of the block to make sure its inspector controls are aligned as desired.

Copy link
Member Author

Choose a reason for hiding this comment

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

Just speed considering all these controls will be superseded by components in following iterations, properly absorbing the styles.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Blocks Overall functionality of blocks [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants