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

Fix: Setting a contrast color, triggers a contrast warning. #14963

Closed
wants to merge 1 commit into from
Closed

Fix: Setting a contrast color, triggers a contrast warning. #14963

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
28 changes: 17 additions & 11 deletions packages/block-library/src/paragraph/edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,17 +33,23 @@ const { getComputedStyle } = window;

const name = 'core/paragraph';

const applyFallbackStyles = withFallbackStyles( ( node, ownProps ) => {
const { textColor, backgroundColor, fontSize, customFontSize } = ownProps.attributes;
const editableNode = node.querySelector( '[contenteditable="true"]' );
//verify if editableNode is available, before using getComputedStyle.
const computedStyles = editableNode ? getComputedStyle( editableNode ) : null;
return {
fallbackBackgroundColor: backgroundColor || ! computedStyles ? undefined : computedStyles.backgroundColor,
fallbackTextColor: textColor || ! computedStyles ? undefined : computedStyles.color,
fallbackFontSize: fontSize || customFontSize || ! computedStyles ? undefined : parseInt( computedStyles.fontSize ) || undefined,
};
} );
const applyFallbackStyles = withFallbackStyles(
( node, ownProps ) => {
const { textColor, backgroundColor, fontSize } = ownProps;
const editableNode = node.querySelector( '[contenteditable="true"]' );
//verify if editableNode is available, before using getComputedStyle.
const computedStyles = editableNode ? getComputedStyle( editableNode ) : null;
return {
fallbackBackgroundColor: ( backgroundColor.color || ! computedStyles ) ? undefined : computedStyles.backgroundColor,
fallbackTextColor: ( textColor.color || ! computedStyles ) ? undefined : computedStyles.color,
fallbackFontSize: ( fontSize.size || ! computedStyles ) ? undefined : parseInt( computedStyles.fontSize ) || undefined,
};
},
( ownProps ) => {
const { textColor, backgroundColor, fontSize } = ownProps;
return [ textColor.color, backgroundColor.color, fontSize.size ];
}
);

class ParagraphBlock extends Component {
constructor() {
Expand Down
23 changes: 20 additions & 3 deletions packages/components/src/higher-order/with-fallback-styles/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,9 @@ import { every, isEqual } from 'lodash';
*/
import { Component } from '@wordpress/element';
import { createHigherOrderComponent } from '@wordpress/compose';
import isShallowEqual from '@wordpress/is-shallow-equal';

export default ( mapNodeToProps ) => createHigherOrderComponent(
export default ( mapNodeToProps, computeWhenChange ) => createHigherOrderComponent(
Copy link
Member

Choose a reason for hiding this comment

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

It is a public API change so we would have to document it and ensure it is backward compatible. It increases the complexity to use this HOC as well.

( WrappedComponent ) => {
return class extends Component {
constructor() {
Expand All @@ -19,6 +20,7 @@ export default ( mapNodeToProps ) => createHigherOrderComponent(
fallbackStyles: undefined,
grabStylesCompleted: false,
};
this.shouldRecomputeValues = this.shouldRecomputeValues().bind( this );

this.bindRef = this.bindRef.bind( this );
}
Expand All @@ -38,9 +40,24 @@ export default ( mapNodeToProps ) => createHigherOrderComponent(
this.grabFallbackStyles();
}

shouldRecomputeValues() {
let lastRecomputeValues = [];
return () => {
if ( ! computeWhenChange ) {
return ! this.state.grabStylesCompleted;
}
const newRecomputeValues = computeWhenChange( this.props );
if ( isShallowEqual( lastRecomputeValues, newRecomputeValues ) ) {
return false;
}
lastRecomputeValues = newRecomputeValues;
return true;
};
}

grabFallbackStyles() {
const { grabStylesCompleted, fallbackStyles } = this.state;
if ( this.nodeRef && ! grabStylesCompleted ) {
const { fallbackStyles } = this.state;
if ( this.nodeRef && this.shouldRecomputeValues() ) {
Copy link
Member

Choose a reason for hiding this comment

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

Is mapNodeToProps that expensive to run that we need to cache it?

It looks like it would solve the issue if we would avoid the local state in the first place. Otherwise, we need to find a way to reset it when the styles change outside, e.g. you apply a class name to the block or change its style variant. which is not taken into account in the proposed solution.

const newFallbackStyles = mapNodeToProps( this.nodeRef, this.props );
if ( ! isEqual( newFallbackStyles, fallbackStyles ) ) {
this.setState( {
Expand Down