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

Jump to bottom

Add props for buttons in editor 2 #65083

Merged
merged 3 commits into from
Sep 12, 2024
Merged

Add props for buttons in editor 2 #65083

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
bc8c7c1
Add __next40pxDefaultSize for editor 2 files
AKSHAT2802 Sep 4, 2024
3187318
Update packages/editor/src/components/post-publish-panel/index.js
AKSHAT2802 Sep 11, 2024
7221a79
update test snapshots
AKSHAT2802 Sep 12, 2024
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
3 changes: 1 addition & 2 deletions packages/editor/src/components/post-format/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,8 +100,7 @@ export default function PostFormat() {
{ suggestion && suggestion.id !== postFormat && (
<p className="editor-post-format__suggestion">
<Button
// TODO: Switch to `true` (40px size) if possible
__next40pxDefaultSize={ false }
__next40pxDefaultSize
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Before After
post format before Post Format after

Copy link
Contributor

Choose a reason for hiding this comment

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

As @mirka mentioned in another PR, variant="link" Button components have their height set to auto, and therefore the __next40pxDefaultSize doesn't make any difference

variant="link"
onClick={ () =>
onUpdatePostFormat( suggestion.id )
Expand Down
3 changes: 1 addition & 2 deletions packages/editor/src/components/post-last-revision/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,7 @@ function PostLastRevision() {
return (
<PostLastRevisionCheck>
<Button
// TODO: Switch to `true` (40px size) if possible
__next40pxDefaultSize={ false }
__next40pxDefaultSize
Copy link
Contributor

Choose a reason for hiding this comment

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

I wasn't able to find how to test this instance in the editor — it looks like the PostLastRevision component is not used anymore since #62323 (cc @ntsekouras ) but we're still exporting it.

In any case, I can see that there are some style overrides in place that tweak the height, and so passing the __next40pxDefaultSize prop shouldn't affect the look of this button.

For the scope of this PR, I think that we can leave things as they are.

As a future improvement, we should avoid overriding button (and panel) styles. If we feel like we're tweaking Button beyond its purpose, we should consider not using Button at all (cc @mirka )

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, this component is not used any more in favor of PrivatePostLastRevision component. There was some discussions whether to remove it here and we ended up keeping it for existing consumers.

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 it still makes sense to remove the overrides if possible.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed — it would be good to do that as a follow up to this PR.

Copy link
Member

Choose a reason for hiding this comment

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

A follow-up PR sounds good 👍

href={ addQueryArgs( 'revision.php', {
revision: lastRevisionId,
} ) }
Expand Down
6 changes: 2 additions & 4 deletions packages/editor/src/components/post-locked-modal/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -257,17 +257,15 @@ export default function PostLockedModal() {
>
{ ! isTakeover && (
<Button
// TODO: Switch to `true` (40px size) if possible
__next40pxDefaultSize={ false }
__next40pxDefaultSize
variant="tertiary"
href={ unlockUrl }
>
{ __( 'Take over' ) }
</Button>
) }
<Button
// TODO: Switch to `true` (40px size) if possible
__next40pxDefaultSize={ false }
__next40pxDefaultSize
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Before After
post lock before Post lock after

variant="primary"
href={ allPostsUrl }
>
Expand Down
3 changes: 1 addition & 2 deletions packages/editor/src/components/post-publish-panel/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,8 +78,7 @@ export class PostPublishPanel extends Component {
<div className="editor-post-publish-panel__header">
{ isPostPublish ? (
<Button
// TODO: Switch to `true` (40px size) if possible
__next40pxDefaultSize={ false }
size="compact"
onClick={ onClose }
icon={ closeSmall }
label={ __( 'Close panel' ) }
Expand Down
3 changes: 1 addition & 2 deletions packages/editor/src/components/post-publish-panel/maybe-post-format-panel.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,7 @@ const PostFormatSuggestion = ( {
onUpdatePostFormat,
} ) => (
<Button
// TODO: Switch to `true` (40px size) if possible
__next40pxDefaultSize={ false }
__next40pxDefaultSize
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Before After
Meybe post format before Maybe Post format after

Copy link
Contributor

Choose a reason for hiding this comment

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

Same as https://github.com/WordPress/gutenberg/pull/65083/files#r1746909466

variant="link"
onClick={ () => onUpdatePostFormat( suggestedPostFormat ) }
>
Expand Down
3 changes: 1 addition & 2 deletions packages/editor/src/components/post-publish-panel/maybe-upload-media.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,8 +155,7 @@ export default function PostFormatPanel() {
<Spinner />
) : (
<Button
// TODO: Switch to `true` (40px size) if possible
__next40pxDefaultSize={ false }
__next40pxDefaultSize
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Before After
Maybe Upload Media before Maybe Upload media after

variant="primary"
onClick={ uploadImages }
>
Expand Down
4 changes: 2 additions & 2 deletions packages/editor/src/components/post-publish-panel/test/__snapshots__/index.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ exports[`PostPublishPanel should render the post-publish panel if the post is pu
>
<button
aria-label="Close panel"
class="components-button has-icon"
class="components-button is-compact has-icon"
type="button"
>
<svg
Expand Down Expand Up @@ -252,7 +252,7 @@ exports[`PostPublishPanel should render the post-publish panel if the post is sc
>
<button
aria-label="Close panel"
class="components-button has-icon"
class="components-button is-compact has-icon"
type="button"
>
<svg
Expand Down
Loading