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

Button: deprecate isPrimary, isSecondary, isTertiary and isLink props in favour of variant prop #31713

Merged
merged 17 commits into from
May 26, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ const ModifiedWarning = ( { originalBlock, ...props } ) => {
originalBlock.title || originalName
);
actions.push(
<Button key="convert" onClick={ convertToHTML } isLink>
<Button key="convert" onClick={ convertToHTML } variant="link">
Copy link
Member

Choose a reason for hiding this comment

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

Wonder if it's slightly odd to see link as a variant. Would it ever make sense to have a "primary link" or a "secondary link"? Should externalLink be considered a variant as well (it exists as a separate thin now)?

Copy link
Contributor

@sarayourfriend sarayourfriend May 21, 2021

Choose a reason for hiding this comment

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

variant="link" doesn't determine whether the component renders an anchor tag, that's determined by the href prop's presence.

Furthermore, the link variant cannot be mixed with the other three variants for a good looking experience. These are the three variants together with link:

Captura de Tela 2021-05-21 às 06 48 34

Captura de Tela 2021-05-21 às 06 48 22

Captura de Tela 2021-05-21 às 06 48 04

They all say "tertiary" button but they're the mixes of link with tertiary, secondary and primary respectively.

Copy link
Member

Choose a reason for hiding this comment

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

If it's determined by href why do we need it as a variant?

Copy link
Contributor

@sarayourfriend sarayourfriend May 21, 2021

Choose a reason for hiding this comment

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

link is a variant of the style of the button, not the functionality. Passing href changes the functionality. You could, for example, have a button that looks like a link by passing <Button onClick={() => alert('hello!')} variant="link">Hello!</Button>

Copy link
Member

@kevin940726 kevin940726 May 24, 2021

Choose a reason for hiding this comment

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

We should probably rename it to text or flat though. material-ui is a good source of inspiration for this. (not necessary related to this PR though)

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd expect text to look most like the variantless button though, i.e., without the underline. 🤷‍♀️ I'm fine renaming it to whatever makes the most sense to people but I don't think we'll find something perfect. Our variants aren't as "varied" as material's. We really just have the five: no variant, primary, secondary, tertiary, and link. And they all or remove the outline/background arbitrarily (link, incidentally, is probably the easiest to guess what it does currently aside from the confusion around functionality).

Choose a reason for hiding this comment

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

I agree. I think it would be best to merge this change (i.e. introducing variant as a preferred alternative to the overlapping is props) and then we could potentially reconsider renaming or changing variants separately. Don't want to keep this PR open any longer because it touches so many usages of Button that it needs constant rebasing, etc.

Copy link
Member

Choose a reason for hiding this comment

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

It's weird to use "link" to refer to an appearance characteristic but we can revisit this separately as we should also clarify the design expectations.

{ __( 'Keep as HTML' ) }
</Button>
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ export default function InstallButton( { attributes, block, clientId } ) {
}
disabled={ isInstallingBlock }
isBusy={ isInstallingBlock }
isPrimary
variant="primary"
>
{ sprintf(
/* translators: %s: block name */
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ function BlockBreadcrumb() {
{ hasSelection && (
<Button
className="block-editor-block-breadcrumb__button"
isTertiary
variant="tertiary"
onClick={ clearSelectedBlock }
>
{ __( 'Document' ) }
Expand All @@ -66,7 +66,7 @@ function BlockBreadcrumb() {
<li key={ parentClientId }>
<Button
className="block-editor-block-breadcrumb__button"
isTertiary
variant="tertiary"
onClick={ () => selectBlock( parentClientId ) }
>
<BlockTitle clientId={ parentClientId } />
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ const BlockView = ( {
</div>

<div className="block-editor-block-compare__action">
<Button isSecondary tabIndex="0" onClick={ action }>
<Button variant="secondary" tabIndex="0" onClick={ action }>
{ actionText }
</Button>
</div>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,9 @@ exports[`BlockView should match snapshot 1`] = `
className="block-editor-block-compare__action"
>
<ForwardRef(Button)
isSecondary={true}
onClick={[Function]}
tabIndex="0"
variant="secondary"
>
action
</ForwardRef(Button)>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ export function BlockInvalidWarning( {
<Button
key="recover"
onClick={ attemptBlockRecovery }
isPrimary
variant="primary"
>
{ __( 'Attempt Block Recovery' ) }
</Button>,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ import { VIEWMODES } from './constants';
const Actions = ( { onStartBlank, onBlockPatternSelect } ) => (
<div className="block-editor-block-pattern-setup__actions">
<Button onClick={ onStartBlank }>{ __( 'Start blank' ) }</Button>
<Button isPrimary onClick={ onBlockPatternSelect }>
<Button variant="primary" onClick={ onBlockPatternSelect }>
{ __( 'Choose' ) }
</Button>
</div>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ function BlockVariationPicker( {
{ variations.map( ( variation ) => (
<li key={ variation.name }>
<Button
isSecondary
variant="secondary"
icon={ variation.icon }
iconSize={ 48 }
onClick={ () => onSelect( variation ) }
Expand All @@ -61,7 +61,7 @@ function BlockVariationPicker( {
{ /* eslint-enable jsx-a11y/no-redundant-roles */ }
{ allowSkip && (
<div className="block-editor-block-variation-picker__skip">
<Button isLink onClick={ () => onSelect() }>
<Button variant="link" onClick={ () => onSelect() }>
{ __( 'Skip' ) }
</Button>
</div>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,9 @@ export default function ImageSizeControl( {
<Button
key={ scale }
isSmall
isPrimary={ isCurrent }
variant={
isCurrent ? 'primary' : undefined
}
isPressed={ isCurrent }
onClick={ updateDimensions(
scaledWidth,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ export default function LinkPreview( { value, onEditClick } ) {
</span>

<Button
isSecondary
variant="secondary"
onClick={ () => onEditClick() }
className="block-editor-link-control__search-item-action"
>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -266,7 +266,7 @@ export function MediaPlaceholder( {
<Button
className="block-editor-media-placeholder__cancel-button"
title={ __( 'Cancel' ) }
isLink
variant="link"
onClick={ onCancel }
>
{ __( 'Cancel' ) }
Expand All @@ -283,7 +283,7 @@ export function MediaPlaceholder( {
className="block-editor-media-placeholder__button"
onClick={ openURLInput }
isPressed={ isURLInputVisible }
isTertiary
variant="tertiary"
>
{ __( 'Insert from URL' ) }
</Button>
Expand Down Expand Up @@ -316,7 +316,7 @@ export function MediaPlaceholder( {
render={ ( { open } ) => {
return (
<Button
isTertiary
variant="tertiary"
onClick={ ( event ) => {
event.stopPropagation();
open();
Expand All @@ -341,7 +341,7 @@ export function MediaPlaceholder( {
const content = (
<>
<Button
isPrimary
variant="primary"
className={ classnames(
'block-editor-media-placeholder__button',
'block-editor-media-placeholder__upload-button'
Expand All @@ -366,7 +366,7 @@ export function MediaPlaceholder( {
<>
{ renderDropZone() }
<FormFileUpload
isPrimary
variant="primary"
className={ classnames(
'block-editor-media-placeholder__button',
'block-editor-media-placeholder__upload-button'
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ export default function PreviewOptions( {
position: 'bottom left',
};
const toggleProps = {
isTertiary: true,
variant: 'tertiary',
className: 'block-editor-post-preview__button-toggle',
disabled: ! isEnabled,
/* translators: button label text should, if possible, be under 16 characters. */
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ const SkipToSelectedBlock = ( { selectedBlockClientId } ) => {

return selectedBlockClientId ? (
<Button
isSecondary
variant="secondary"
className="block-editor-skip-to-selected-block"
onClick={ onClick }
>
Expand Down
2 changes: 1 addition & 1 deletion packages/block-editor/src/hooks/layout.js
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ function LayoutPanel( { setAttributes, attributes } ) {
</div>
<div className="block-editor-hooks__layout-controls-reset">
<Button
isSecondary
variant="secondary"
isSmall
disabled={ ! contentSize && ! wideSize }
onClick={ () =>
Expand Down
6 changes: 5 additions & 1 deletion packages/block-library/src/button/edit.js
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,11 @@ function WidthPanel( { selectedWidth, setAttributes } ) {
<Button
key={ widthValue }
isSmall
isPrimary={ widthValue === selectedWidth }
variant={
widthValue === selectedWidth
? 'primary'
: undefined
}
onClick={ () => handleChange( widthValue ) }
>
{ widthValue }%
Expand Down
2 changes: 1 addition & 1 deletion packages/block-library/src/cover/edit.js
Original file line number Diff line number Diff line change
Expand Up @@ -492,7 +492,7 @@ function CoverEdit( {
) }
<PanelRow>
<Button
isSecondary
variant="secondary"
isSmall
className="block-library-cover__reset-button"
onClick={ () =>
Expand Down
6 changes: 3 additions & 3 deletions packages/block-library/src/embed/embed-placeholder.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ const EmbedPlaceholder = ( {
placeholder={ __( 'Enter URL to embed here…' ) }
onChange={ onChange }
/>
<Button isPrimary type="submit">
<Button variant="primary" type="submit">
{ _x( 'Embed', 'button label' ) }
</Button>
</form>
Expand All @@ -51,10 +51,10 @@ const EmbedPlaceholder = ( {
<div className="components-placeholder__instructions">
{ __( 'Sorry, this content could not be embedded.' ) }
</div>
<Button isSecondary onClick={ tryAgain }>
<Button variant="secondary" onClick={ tryAgain }>
{ _x( 'Try again', 'button label' ) }
</Button>{ ' ' }
<Button isSecondary onClick={ fallback }>
<Button variant="secondary" onClick={ fallback }>
{ _x( 'Convert to link', 'button label' ) }
</Button>
</div>
Expand Down
2 changes: 1 addition & 1 deletion packages/block-library/src/missing/edit.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ function MissingBlockWarning( { attributes, convertToHTML } ) {
originalName
);
actions.push(
<Button key="convert" onClick={ convertToHTML } isPrimary>
<Button key="convert" onClick={ convertToHTML } variant="primary">
{ __( 'Keep as HTML' ) }
</Button>
);
Expand Down
10 changes: 6 additions & 4 deletions packages/block-library/src/navigation/placeholder.js
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ function NavigationPlaceholder( { onCreate }, ref ) {
}, [ isCreatingFromMenu, hasResolvedMenuItems ] );

const toggleProps = {
isPrimary: true,
variant: 'primary',
className: 'wp-block-navigation-placeholder__actions__dropdown',
};
return (
Expand Down Expand Up @@ -132,14 +132,16 @@ function NavigationPlaceholder( { onCreate }, ref ) {
) : undefined }
{ hasPages ? (
<Button
isPrimary={ hasMenus ? false : true }
isTertiary={ hasMenus ? true : false }
variant={ hasMenus ? 'tertiary' : 'primary' }
onClick={ onCreateAllPages }
>
{ __( 'Add all pages' ) }
</Button>
) : undefined }
<Button isTertiary onClick={ onCreateEmptyMenu }>
<Button
variant="tertiary"
onClick={ onCreateEmptyMenu }
>
{ __( 'Start empty' ) }
</Button>
</div>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -103,11 +103,11 @@ export default function ConvertToLinksModal( { onClose, clientId } ) {
) }
</p>
<div className="wp-block-page-list-modal-buttons">
<Button isTertiary onClick={ onClose }>
<Button variant="tertiary" onClick={ onClose }>
{ __( 'Cancel' ) }
</Button>
<Button
isPrimary
variant="primary"
disabled={ ! pagesFinished }
onClick={ convertSelectedBlockToNavigationLinks( {
pages,
Expand Down
2 changes: 1 addition & 1 deletion packages/block-library/src/post-comment/edit.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ export default function Edit( { attributes, setAttributes } ) {
/>

<Button
isPrimary
variant="primary"
onClick={ () => {
setAttributes( { commentId: commentIdInput } );
} }
Expand Down
2 changes: 1 addition & 1 deletion packages/block-library/src/rss/edit.js
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ export default function RSSEdit( { attributes, setAttributes } ) {
}
className="wp-block-rss__placeholder-input"
/>
<Button isPrimary type="submit">
<Button variant="primary" type="submit">
{ __( 'Use URL' ) }
</Button>
</form>
Expand Down
4 changes: 3 additions & 1 deletion packages/block-library/src/search/edit.js
Original file line number Diff line number Diff line change
Expand Up @@ -295,9 +295,11 @@ export default function SearchEdit( {
<Button
key={ widthValue }
isSmall
isPrimary={
variant={
`${ widthValue }%` ===
`${ width }${ widthUnit }`
? 'primary'
: undefined
}
onClick={ () =>
setAttributes( {
Expand Down
2 changes: 1 addition & 1 deletion packages/block-library/src/table/edit.js
Original file line number Diff line number Diff line change
Expand Up @@ -528,7 +528,7 @@ function TableEdit( {
/>
<Button
className="blocks-table__placeholder-button"
isPrimary
variant="primary"
type="submit"
>
{ __( 'Create Table' ) }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -107,17 +107,17 @@ export default function TemplatePartPlaceholder( {
<>
{ enableSelection && (
<Button
isPrimary
variant="primary"
onClick={ onToggle }
aria-expanded={ isOpen }
>
{ __( 'Choose existing' ) }
</Button>
) }
<Button
{ ...( enableSelection
? { isTertiary: true }
: { isPrimary: true } ) }
variant={
enableSelection ? 'tertiary' : 'primary'
}
onClick={ () =>
setStep( PLACEHOLDER_STEPS.patterns )
}
Expand Down
7 changes: 5 additions & 2 deletions packages/block-library/src/video/edit.js
Original file line number Diff line number Diff line change
Expand Up @@ -199,7 +199,7 @@ function VideoEdit( {
}
render={ ( { open } ) => (
<Button
isPrimary
variant="primary"
onClick={ open }
ref={ posterImageButton }
aria-describedby={
Expand All @@ -226,7 +226,10 @@ function VideoEdit( {
) }
</p>
{ !! poster && (
<Button onClick={ onRemovePoster } isTertiary>
<Button
onClick={ onRemovePoster }
variant="tertiary"
>
{ __( 'Remove' ) }
</Button>
) }
Expand Down
6 changes: 3 additions & 3 deletions packages/block-library/src/video/tracks-editor.js
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ function TrackList( { tracks, onEditPress } ) {
>
<span>{ track.label } </span>
<Button
isTertiary
variant="tertiary"
onClick={ () => onEditPress( index ) }
aria-label={ sprintf(
/* translators: %s: Label of the video text track e.g: "French subtitles" */
Expand Down Expand Up @@ -152,7 +152,7 @@ function SingleTrackEditor( { track, onChange, onClose, onRemove } ) {
/>
<div className="block-library-video-tracks-editor__single-track-editor-buttons-container">
<Button
isSecondary
variant="secondary"
onClick={ () => {
const changes = {};
let hasChanges = false;
Expand All @@ -179,7 +179,7 @@ function SingleTrackEditor( { track, onChange, onClose, onRemove } ) {
>
{ __( 'Close' ) }
</Button>
<Button isDestructive isLink onClick={ onRemove }>
<Button isDestructive variant="link" onClick={ onRemove }>
{ __( 'Remove track' ) }
</Button>
</div>
Expand Down
Loading