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

Inspector Controls: Rephrase, polish, and make consistent color labels. #30075

Merged
merged 2 commits into from
Apr 15, 2021
Merged

Inspector Controls: Rephrase, polish, and make consistent color labels. #30075

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1bc26b8
Try: Rephrase, polish, and make consistent color labels.
jasmussen Mar 22, 2021
37186a9
Restore background color.
jasmussen Mar 22, 2021
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
4 changes: 2 additions & 2 deletions lib/block-supports/colors.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ function gutenberg_apply_colors_support( $block_type, $block_attributes ) {
$classes = array();
$styles = array();

// Text Colors.
// Text colors.
// Check support for text colors.
if ( $has_text_colors_support ) {
$has_named_text_color = array_key_exists( 'textColor', $block_attributes );
Expand Down Expand Up @@ -117,7 +117,7 @@ function gutenberg_apply_colors_support( $block_type, $block_attributes ) {
}
}

// Background Colors.
// Background colors.
if ( $has_background_colors_support ) {
$has_named_background_color = array_key_exists( 'backgroundColor', $block_attributes );
$has_custom_background_color = isset( $block_attributes['style']['color']['background'] );
Expand Down
8 changes: 4 additions & 4 deletions packages/block-editor/src/components/colors/use-colors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,8 @@ function getComputedStyle( node ) {
const DEFAULT_COLORS = [];

const COMMON_COLOR_LABELS = {
textColor: __( 'Text Color' ),
backgroundColor: __( 'Background Color' ),
textColor: __( 'Text color' ),
backgroundColor: __( 'Background color' ),
};

const InspectorControlsColorPanel = ( props ) => (
Expand All @@ -48,7 +48,7 @@ const InspectorControlsColorPanel = ( props ) => (
export default function __experimentalUseColors(
colorConfigs,
{
panelTitle = __( 'Color settings' ),
panelTitle = __( 'Color' ),
colorPanelProps,
contrastCheckers,
panelChildren,
Expand All @@ -58,7 +58,7 @@ export default function __experimentalUseColors(
textColorTargetRef = targetRef,
} = {},
} = {
panelTitle: __( 'Color settings' ),
panelTitle: __( 'Color' ),
},
deps = []
) {
Expand Down
2 changes: 1 addition & 1 deletion packages/block-editor/src/hooks/color-panel.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ export default function ColorPanel( {
return (
<InspectorControls>
<PanelColorGradientSettings
title={ __( 'Color settings' ) }
title={ __( 'Color' ) }
initialOpen={ false }
settings={ settings }
>
Expand Down
4 changes: 2 additions & 2 deletions packages/block-editor/src/hooks/color.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -327,7 +327,7 @@ export function ColorEdit( props ) {
...( hasTextColorSupport( blockName )
? [
{
label: __( 'Text Color' ),
label: __( 'Text color' ),
onColorChange: onChangeColor( 'text' ),
colorValue: getColorObjectByAttributeValues(
colors,
Expand All @@ -340,7 +340,7 @@ export function ColorEdit( props ) {
...( hasBackground || hasGradient
? [
{
label: __( 'Background Color' ),
label: __( 'Background color' ),
onColorChange: hasBackground
? onChangeColor( 'background' )
: undefined,
Expand Down
8 changes: 3 additions & 5 deletions packages/block-library/src/button/color-edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,9 +68,7 @@ function ColorPanel( { settings, clientId, enableContrastChecking = true } ) {
const [ detectedBackgroundColor, setDetectedBackgroundColor ] = useState();
const [ detectedColor, setDetectedColor ] = useState();

const title = isWebPlatform
? __( 'Color settings' )
: __( 'Color Settings' );
Copy link
Member

Choose a reason for hiding this comment

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

@hypest, is there any reason this differs in the mobile app. Are there tests that depend on it that need to get updated?

Copy link
Contributor

Choose a reason for hiding this comment

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

As far as I checked, we don't have any test that relies on this. I checked the commit that introduced this change and it doesn't look like the decision of using different values has to do beyond a naming convention. Being this said, I'd say it's safe to continue with this change 👍 .

@hypest do you agree?

Copy link
Contributor

@hypest hypest Apr 14, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Yeap, I agree. To be on the safe side I created this gutenberg-mobile PR to run all tests from the native mobile side of things. Edit: native mobile tests are green 👍.

Copy link
Contributor

Choose a reason for hiding this comment

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

👋 @iamthomasbishop , what's your take on the use of Sentence case here? Will it be a problem for the mobile apps? See for example this diff.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another way to think about it is: is title-case worth extra code across the entire project?

Copy link
Member

Choose a reason for hiding this comment

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

In Polish you don't even have the choice, it's a sentence case on all websites, sometimes they use uppercase for highlights 😄

Copy link

Choose a reason for hiding this comment

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

what's your take on the use of Sentence case here

@hypest We're slowly moving towards sentence-casing on mobile, so we can remove the mobile-specific code here.

Copy link
Contributor

Choose a reason for hiding this comment

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

✅ all systems go then! I will go ahead and merge this PR, thank you all!

const title = __( 'Color' );

useEffect( () => {
if ( isWebPlatform && ! enableContrastChecking ) {
Expand Down Expand Up @@ -209,7 +207,7 @@ function ColorEdit( props ) {
const settings = useMemo( () => {
return [
{
label: __( 'Text Color' ),
label: __( 'Text color' ),
onColorChange: onChangeColor( 'text' ),
colorValue: getColorObjectByAttributeValues(
colors,
Expand All @@ -218,7 +216,7 @@ function ColorEdit( props ) {
).color,
},
{
label: __( 'Background Color' ),
label: __( 'Background color' ),
onColorChange: onChangeColor( 'background' ),
colorValue: getColorObjectByAttributeValues(
colors,
Expand Down
2 changes: 1 addition & 1 deletion packages/block-library/src/pullquote/edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,7 @@ function PullQuoteEdit( {
{ Platform.OS === 'web' && (
<InspectorControls>
<PanelColorSettings
title={ __( 'Color settings' ) }
title={ __( 'Color' ) }
colorSettings={ [
{
value: mainColor.color,
Expand Down
2 changes: 1 addition & 1 deletion packages/block-library/src/separator/separator-settings.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import { InspectorControls, PanelColorSettings } from '@wordpress/block-editor';
const SeparatorSettings = ( { color, setColor } ) => (
<InspectorControls>
<PanelColorSettings
title={ __( 'Color settings' ) }
title={ __( 'Color' ) }
colorSettings={ [
{
value: color.color,
Expand Down
4 changes: 2 additions & 2 deletions packages/block-library/src/social-links/edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,7 @@ export function SocialLinksEdit( props ) {
/>
</PanelBody>
<PanelColorSettings
title={ __( 'Color settings' ) }
title={ __( 'Color' ) }
colorSettings={ [
{
// Use custom attribute as fallback to prevent loss of named color selection when
Expand All @@ -205,7 +205,7 @@ export function SocialLinksEdit( props ) {
iconBackgroundColorValue: colorValue,
} );
},
label: __( 'Icon background color' ),
label: __( 'Background color' ),
},
] }
/>
Expand Down
2 changes: 1 addition & 1 deletion packages/block-library/src/table/edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -500,7 +500,7 @@ function TableEdit( {
/>
</PanelBody>
<PanelColorSettings
title={ __( 'Color settings' ) }
title={ __( 'Color' ) }
initialOpen={ false }
colorSettings={ [
{
Expand Down
2 changes: 1 addition & 1 deletion packages/e2e-tests/specs/editor/blocks/heading.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ describe( 'Heading', () => {
const COLOR_INPUT_FIELD_SELECTOR =
'.components-color-palette__picker .components-text-control__input';
const COLOR_PANEL_TOGGLE_X_SELECTOR =
"//button[./span[contains(text(),'Color settings')]]";
"//button[./span[contains(text(),'Color')]]";

beforeEach( async () => {
await createNewPost();
Expand Down