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

Font family preview in the font family picker #67118

Merged
merged 8 commits into from
Nov 22, 2024
22 changes: 12 additions & 10 deletions packages/block-editor/src/components/font-family/index.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/**
* WordPress dependencies
*/
import { SelectControl } from '@wordpress/components';
import { CustomSelectControl } from '@wordpress/components';
import deprecated from '@wordpress/deprecated';
import { __ } from '@wordpress/i18n';

Expand Down Expand Up @@ -30,13 +30,15 @@ export default function FontFamilyControl( {
}

const options = [
{ value: '', label: __( 'Default' ) },
...fontFamilies.map( ( { fontFamily, name } ) => {
return {
value: fontFamily,
label: name || fontFamily,
};
} ),
{
key: '',
name: __( 'Default' ),
},
...fontFamilies.map( ( { fontFamily, name } ) => ( {
key: fontFamily,
name: name || fontFamily,
style: { fontFamily },
} ) ),
];

if ( ! __nextHasNoMarginBottom ) {
Expand All @@ -51,14 +53,14 @@ export default function FontFamilyControl( {
}

return (
<SelectControl
<CustomSelectControl
__next40pxDefaultSize={ __next40pxDefaultSize }
__nextHasNoMarginBottom={ __nextHasNoMarginBottom }
Copy link
Member

Choose a reason for hiding this comment

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

I think you need to remove this prop.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure what prop are you pointing to.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you mean __next40pxDefaultSize, I see it used in other parts of the codebase where CustomSelectControl is rendered, too:

__next40pxDefaultSize={ __next40pxDefaultSize }

Copy link
Member

Choose a reason for hiding this comment

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

My comment is referring to the __nextHasNoMarginBottom prop, actually.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, removed 1f94e6c

label={ __( 'Font' ) }
options={ options }
value={ value }
onChange={ onChange }
matiasbenedetto marked this conversation as resolved.
Show resolved Hide resolved
labelPosition="top"
Copy link
Member

Choose a reason for hiding this comment

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

I think this prop also should be removed. Aren't you getting some errors in the console when passing this prop? I think you should, since it will end up being wrongly passed down to the underlying DOM element.

Copy link
Contributor Author

@matiasbenedetto matiasbenedetto Nov 21, 2024

Choose a reason for hiding this comment

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

It looks like we are passing the options as described in the component docs. See the options description and data example in storybook: https://wordpress.github.io/gutenberg/?path=/docs/components-customselectcontrol--docs

Copy link
Contributor Author

@matiasbenedetto matiasbenedetto Nov 21, 2024

Choose a reason for hiding this comment

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

I don't see console errors and it is used like that in other parts of the codebase. Example:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe you are referring something that is valid for the CustomSelectControlV2 ? I saw that control in the codebase, but it's not listed on storybook, and it doesn't seem to be in use in the Gutenberg codebase, so I'm not sure what it's their current state.

Copy link
Member

Choose a reason for hiding this comment

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

My comment is referring to the labelPosition prop, not the options prop.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed here: 1f94e6c

options={ options }
{ ...props }
/>
);
Expand Down
Loading