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

Fix: Align: Only add data-align for wide/full aligns if editor/theme supports them #9481

Merged
merged 2 commits into from Nov 19, 2018

Conversation

Projects
None yet
6 participants
@jorgefilipecosta
Member

jorgefilipecosta commented Aug 30, 2018

Full and wide aligns block alignments were always shown in the editor provided they were previously set or set because of a default value even if the current theme does not support them.

This PR does not make the editor change anything that has been saved. It just updates the editor to not show wide and full alignments on existing blocks (and default attributes) when they are not supported by the current theme so what the user sees in backend matches the front-end.
Unless explicitly changed by the user the aligns continue to be wide/full and if later the user changes to a theme that supports this aligns the editor will show again wide/full without any change.

The wide and full alignments were already not being shown in the Block alignment toolbar if the theme does not support them.

How has this been tested?

I added the following test block https://gist.github.com/jorgefilipecosta/786b534ab2d4432227d9adc346eb5ad2.
I verified that when I add it to themes that support wide/full aligns the block is created with wide alignment and on themes with no wide/full support the block is created without wide alignment.

Add the "Media & Text" block verify in the "Twenty Fourteen" theme by default when the block is inserted it is at a normal width. Repeat the test in "Twenty Nineteen" or "Gutenberg starter theme" and verify by default the block has a wide width.

@jorgefilipecosta jorgefilipecosta self-assigned this Aug 30, 2018

@jorgefilipecosta jorgefilipecosta force-pushed the fix/align-does-not-checks-if-wide-and-full-alignments-suppoorted-on-editor-context branch from 4ec83cb to 083eb09 Aug 30, 2018

@ZebulanStanphill

This comment has been minimized.

Contributor

ZebulanStanphill commented Aug 31, 2018

Just to be clear, this does not change the alignment setting of blocks already placed on the page, does it?

@jorgefilipecosta

This comment has been minimized.

Member

jorgefilipecosta commented Aug 31, 2018

Hi @ZebulanStanphill,

Just to be clear, this does not change the alignment setting of blocks already placed on the page, does it?

No, it does not change anything that has been saved. It just updates the editor to not show wide and full alignments on existing blocks (and default attributes) when they are not supported by the current theme so what the user sees in backend matches the front-end.
Unless explicitly changed by the user the aligns continue to be wide/full and if later the user changes to a theme that supports this aligns the editor will show again wide/full without any change.
(added to the PR description)

@ZebulanStanphill

This comment has been minimized.

Contributor

ZebulanStanphill commented Aug 31, 2018

Good to know! Knowing that, I think this is a good idea. 👍

@jorgefilipecosta jorgefilipecosta force-pushed the fix/align-does-not-checks-if-wide-and-full-alignments-suppoorted-on-editor-context branch 3 times, most recently from 0b7498f to 20435dc Aug 31, 2018

@mtias mtias referenced this pull request Sep 1, 2018

Open

🖍 Styling #9534

@jorgefilipecosta jorgefilipecosta force-pushed the fix/align-does-not-checks-if-wide-and-full-alignments-suppoorted-on-editor-context branch 2 times, most recently from 7ec766d to 1b42e0d Sep 6, 2018

@jorgefilipecosta jorgefilipecosta requested a review from WordPress/gutenberg-core Sep 7, 2018

@jorgefilipecosta jorgefilipecosta added this to the 4.4 milestone Nov 8, 2018

@jorgefilipecosta jorgefilipecosta force-pushed the fix/align-does-not-checks-if-wide-and-full-alignments-suppoorted-on-editor-context branch from 1b42e0d to dd58860 Nov 8, 2018

@jorgefilipecosta

This comment has been minimized.

Member

jorgefilipecosta commented Nov 8, 2018

This PR was rebased and updated. It was added to the 4.4 milestone (priority increased) because now it addresses a problem that is affecting users.
The "Media & Text" block appears wide by default even if the theme does not support a wide alignment, this PR should solve that problem.

@@ -109,10 +115,13 @@ export const withToolbarControls = createHigherOrderComponent( ( BlockEdit ) =>
* @param {Function} BlockListBlock Original component
* @return {Function} Wrapped component
*/
export const withDataAlign = createHigherOrderComponent( ( BlockListBlock ) => {
export const innerWithDataAlign = createHigherOrderComponent( ( BlockListBlock ) => {

This comment has been minimized.

@aduth

aduth Nov 12, 2018

Member

inner is a pretty superfluous prefix. We should have names for any createHigherOrderComponent too. I think we can avoid it anyways?

export const withDataAlign = createHigherOrderComponent( ( BlockListBlock ) => compose( [
	withSelect( /* ... */ ),
	( props ) => { /* ... */ }
] ) );

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Nov 13, 2018

Member

Hi @aduth, I tried to apply your suggestion, although I still need to export innerWithDataAlign inside the module for testing purposes. Now, I'm not using createHigherOrderComponent in the inner component.
I agree the inner prefix is superfluous but to be honest I don't have a better idea for this case.
The other idea I had was on the withSelect to use hasWideEnabled prop if it was passed, so we can use the prop in our tests, but I thought the prop may end up being used in ways we are expecting and decided to not use this logic.

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Nov 13, 2018

Member

Update: I renamed to insideSelectWithDataAlign at least it specifies what will be the "container", still not a very good name but at least adds some information.

* @param {string} blockName Block name to check
* @return {string[]} Valid alignments for block
* @param {string} blockName Block name to check
* @param {?boolean} considerWideControlsEnabled True if the function should consider wide and full alignments as supported.

This comment has been minimized.

@aduth

aduth Nov 12, 2018

Member

Does the name need to be so verbose? Could something like hasWideControls or hasWideSupport or hasWideEnabled suffice?

This comment has been minimized.

@aduth

aduth Nov 12, 2018

Member

Another worry here is considering the distinction of "wide enablers", since we have both block support and theme support, one coming through arguments and the other determined in the logic of the function itself. Ideally these weren't disjointed. Technically both are available via the data module, so I could see either both being arguments or both being selected.

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Nov 13, 2018

Member

I tried to have a selector that given a block name returns the valid alignments. But there are some edge cases, (e.g., during the save hook we always consider the theme allows wide alignments so changing theme does not impact the save), I concluded the selector would be too specific, so I reverted and followed the path of both "wide enablers" being arguments.

// Explicitly defined array set of valid alignments
const blockAlign = getBlockSupport( blockName, 'align' );
if ( Array.isArray( blockAlign ) ) {
// remove wide and full from the array of alignments if theme does not supports them.
if ( ! considerWideControlsEnabled ) {

This comment has been minimized.

@aduth

aduth Nov 12, 2018

Member

Kinda seems like we could reorganize the function to avoid considering this twice.

export function getBlockValidAlignments( blockName, hasWideEnabled = true ) {
	let validAlignments;

	const blockAlign = getBlockSupport( blockName, 'align' );
	if ( Array.isArray( blockAlign ) ) {
		// Explicitly defined array set of valid alignments
		validAlignments = blockAlign;
		return blockAlign;
	} else if ( blockAlign === true ) {
		// `true` includes all alignments...
		validAlignments = [ 'left', 'center', 'right' ];

		if ( hasBlockSupport( blockName, 'alignWide', true ) ) {
			validAlignments.push( 'wide', 'full' );
		}
	} else {
		validAlignments = [];
	}

	// Remove wide and full if not supported by theme.
	if ( ! hasWideEnabled ) {
		return reject( blockAlign, ( align ) => (
			align === 'wide' ||
			align === 'full'
		) );
	}

	return validAlignments;
}

This comment has been minimized.

@aduth

aduth Nov 12, 2018

Member

In-fact, this makes me think: How do we handle the case where align is specified as an array, but alignWide is not also provided as a supports? Would that be something like:

export function getBlockValidAlignments( blockName, hasWideEnabled = true ) {
	let validAlignments;

	const blockAlign = getBlockSupport( blockName, 'align' );
	if ( Array.isArray( blockAlign ) ) {
		// Explicitly defined array set of valid alignments
		validAlignments = blockAlign;
		return blockAlign;
	} else if ( blockAlign === true ) {
		// `true` includes all alignments...
		validAlignments = [ 'left', 'center', 'right', 'wide', 'full' ];
	} else {
		validAlignments = [];
	}

	// Remove wide and full if not supported by theme.
	const hasWideBlockSupport = hasBlockSupport( blockName, 'alignWide', true );
	if ( ! hasWideEnabled || ! hasWideBlockSupport ) {
		return reject( blockAlign, ( align ) => (
			align === 'wide' ||
			align === 'full'
		) );
	}

	return validAlignments;
}

It might be strange to reject wide and full if the block explicitly assigns them as align anyways, but it becomes more a question then if we could even rely on alignWide supports verbatim if it's not explicitly provided by the block definition. This way it becomes more obviously problematic to the block implementer that they'd need to provide the value.

Alternatively, we could enhance hasBlockSupport for the 'alignWide' key to consider the array-form of align and whether it includes the 'wide' and 'full' values.

Thoughts?

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Nov 13, 2018

Member

Hi, @aduth I applied a huge refactor to the function taking into account your suggestions.
To note that, if the theme does not support wide alignments they are ignored in the hook even if they were set using an array. While the block support for wide alignments does not apply if the block set the align using an array.

I followed this rule because currently, the toolbar removes the wide align buttons if the theme does not support wide alignment (even if the align was set using an array). But if an align previously existed it was set anyway in the editor; this caused a bug:
Use a theme that supports wide aligns.
Create a post add a columns block, set full alignment.
Use a theme that does not support wide aligns.
Open the previous post, see that the columns contain full alignment and the wide align buttons are not shown so we can not disable this alignment even if the theme does not support it.

The idea of this PR is to make sure withDataAlign follows the same rules already being followed by the alignment toolbar.

return ( props ) => {
const { align } = props.block.attributes;
const validAlignments = getBlockValidAlignments( props.block.name );
const validAlignments = getBlockValidAlignments(

This comment has been minimized.

@aduth

aduth Nov 12, 2018

Member

We didn't update the call to getBlockValidAlignments in withToolbarControls. Do we need to?

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Nov 13, 2018

Member

We don't need to, the BlockAlignmentToolbar component already checks for wide support in theme and removes wide alignments if there are not supported https://github.com/WordPress/gutenberg/blob/master/packages/editor/src/components/block-alignment-toolbar/index.js#L46.
So doing the check here would just make the check two times.
I thought about removing the check from BlockAlignmentToolbar, but the block can use the component directly without the hook and doing this we would be changing the component behavior which I'm not sure if it is something we can do now.
So I preferred to keep the check there.

@jorgefilipecosta jorgefilipecosta force-pushed the fix/align-does-not-checks-if-wide-and-full-alignments-suppoorted-on-editor-context branch 3 times, most recently from d48b441 to 9224cd9 Nov 12, 2018

@jorgefilipecosta

This comment has been minimized.

Member

jorgefilipecosta commented Nov 13, 2018

Thank you for review @aduth, I tried to apply your feedback.

@jorgefilipecosta jorgefilipecosta force-pushed the fix/align-does-not-checks-if-wide-and-full-alignments-suppoorted-on-editor-context branch from 9224cd9 to 5189dc1 Nov 13, 2018

@youknowriad youknowriad modified the milestones: 4.4, 4.5 Nov 15, 2018

@gziolo gziolo added the Priority Low label Nov 19, 2018

@aduth

I don't know that this is doing what it's claiming to do? For a post in which I'd previously saved a wide-aligned block, then changed to a theme without wide support, I still see the data-align="wide" despite the alignment options not being reflected in the shown toolbar.

Is that expected?

image


/**
* Internal dependencies
*/
import { BlockControls, BlockAlignmentToolbar } from '../components';

/**
* Internal Constants

This comment has been minimized.

@aduth

aduth Nov 19, 2018

Member

I find these blocks encourage a bad practice (avoiding to document constants properly) and would be better off completely omitted than to exist at all. The best case would be to document each constant.


if (
! hasWideEnabled ||
( blockAlign === true && ! hasWideBlockSupport )

This comment has been minimized.

@aduth

aduth Nov 19, 2018

Member

Stylistically, this should be one or the other of:

if (
	! hasWideEnabled ||
	( blockAlign === true && ! hasWideBlockSupport )
) {
	// ...
}
if ( ! hasWideEnabled ||
		( blockAlign === true && ! hasWideBlockSupport ) ) {
	// ...
}

Current is a strange mix of both.

*
* @return {string[]} Valid alignments.
*/
export const getValidAlignments = ( blockAlign, hasWideBlockSupport = true, hasWideEnabled = true ) => {

This comment has been minimized.

@aduth

aduth Nov 19, 2018

Member

I can't help but be curious why this was changed from function keyword to an arrow function.

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Nov 19, 2018

Member

I don't have any reason for this change, during my work in this PR I tried another approach (a cached selector) that required an arrow function when reverting back I kept the arrow function.
I reverted back this change as it was not necessary.

const { name: blockName } = props;
// Compute valid alignments without taking into account,
// if the theme supports wide alignments or not.
// BlockAlignmentToolbar takes into account the theme support.

This comment has been minimized.

@aduth

aduth Nov 19, 2018

Member

It seems strange to me we'd want to fragment the logic on this. Why not eliminate what exists today in BlockAlignmentToolbar and consolidate it here?

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Nov 19, 2018

Member

The reason is that I'm not sure we can not remove this checks from BlockAlignmentToolbar, this component may be used by external blocks directly and they may depend on the existing behavior of this component.
We depend on it for some of our own blocks, e.g. the image.

( select ) => {
const { getEditorSettings } = select( 'core/editor' );
return {
hasWideEnabled: getEditorSettings().alignWide,

This comment has been minimized.

@aduth

aduth Nov 19, 2018

Member

Do we have any confidence in knowing that the type of alignWide will be a boolean here (e.g. if undefined) ? I'd tend toward coercing !! value if how we're passing it along is said to be of boolean form.

@jorgefilipecosta

This comment has been minimized.

Member

jorgefilipecosta commented Nov 19, 2018

I don't know that this is doing what it's claiming to do? For a post in which I'd previously saved a wide-aligned block, then changed to a theme without wide support, I still see the data-align="wide" despite the alignment options not being reflected in the shown toolbar.

Is that expected?

Unfortunately, it looks like the image blcok, does not uses the supports align, it implements everything on the block. So it is expected. This PR just applies this change for blocks that use our supports align mechanism (pull quote, gallery, cover, audio, media & text etc...).
I'm not sure why in the image case the supports align is not used, I think it should, but let's leave that refactor for a different PR.

Fix: Align: Only add data-align for wide and full aligns if editor/th…
…eme supports them

Full and wide aligns were always shown in the editor if they were previously set or set because of a default value even if the current theme does not supports them.
@aduth

aduth approved these changes Nov 19, 2018

@aduth

This comment has been minimized.

Member

aduth commented Nov 19, 2018

Behaves better when testing with Pullquote block.

@jorgefilipecosta jorgefilipecosta force-pushed the fix/align-does-not-checks-if-wide-and-full-alignments-suppoorted-on-editor-context branch from 5189dc1 to 1269a49 Nov 19, 2018

@catehstn

This comment has been minimized.

catehstn commented Nov 19, 2018

@jorgefilipecosta how close is this to being mergeable? Do we need to push it to 4.6?

@jorgefilipecosta jorgefilipecosta merged commit 300a2de into master Nov 19, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jorgefilipecosta jorgefilipecosta deleted the fix/align-does-not-checks-if-wide-and-full-alignments-suppoorted-on-editor-context branch Nov 19, 2018

@jorgefilipecosta

This comment has been minimized.

Member

jorgefilipecosta commented Nov 19, 2018

Hi @aduth thank you a lot for your reviews your feedback was applied/answered.

Hi @catehstn, It is merged now, I was just waiting for the green light from our CI tests.

grey-rsi pushed a commit to OnTheGoSystems/gutenberg that referenced this pull request Nov 22, 2018

Fix: Align: Only add data-align for wide/full aligns if editor/theme …
…supports them (WordPress#9481)

Full and wide aligns block alignments were always shown in the editor provided they were previously set or set because of a default value even if the current theme does not support them.

This PR does not make the editor change anything that has been saved. It just updates the editor to not show wide and full alignments on existing blocks (and default attributes) when they are not supported by the current theme so what the user sees in backend matches the front-end.
Unless explicitly changed by the user the aligns continue to be wide/full and if later the user changes to a theme that supports this aligns the editor will show again wide/full without any change.

The wide and full alignments were already not being shown in the Block alignment toolbar if the theme does not support them.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment