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 1 #65068

Merged
merged 4 commits into from
Sep 12, 2024
Merged

Add props for buttons in editor 1 #65068

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
42d5cca
Add props for buttons in editor 1
AKSHAT2802 Sep 4, 2024
014f941
Merge branch 'trunk' into fix/button-size-violation-editor-1
AKSHAT2802 Sep 4, 2024
4bef78a
Address feedback
AKSHAT2802 Sep 11, 2024
55ac540
remove extra class name and extra styles
AKSHAT2802 Sep 11, 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/block-manager/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,8 +67,7 @@ function BlockManager( {
numberOfHiddenBlocks
) }
<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

Style didn't change because there are explicit styles for Reset in code. But adding __next40pxDefaultSize will ensure that it will not be listed in lint next time

Copy link
Member

Choose a reason for hiding this comment

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

Note for posterity: There are no explicit height styles for this, just that "link" variant Buttons have height: auto applied.

Copy link
Contributor

Choose a reason for hiding this comment

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

😅

variant="link"
onClick={ () => enableAllBlockTypes( blockTypes ) }
>
Expand Down
3 changes: 1 addition & 2 deletions packages/editor/src/components/editor-history/redo.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,7 @@ function EditorHistoryRedo( props, ref ) {
const { redo } = useDispatch( editorStore );
return (
<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
undo-redo before undo-redo after

Style didn't change because there are explicit styles for Undo and Redo buttons in code. But adding __next40pxDefaultSize will ensure that it will not be listed in lint next time

Copy link
Member

Choose a reason for hiding this comment

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

Note for posterity: In our app code, sizes are added by the consumers.

gutenberg/packages/editor/src/components/document-tools/index.js

Lines 151 to 156 in 9523fb4

<ToolbarItem
as={ EditorHistoryUndo }
showTooltip={ ! showIconLabels }
variant={ showIconLabels ? 'tertiary' : undefined }
size="compact"
/>

{ ...props }
ref={ ref }
icon={ ! isRTL() ? redoIcon : undoIcon }
Expand Down
3 changes: 1 addition & 2 deletions packages/editor/src/components/editor-history/undo.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,7 @@ function EditorHistoryUndo( props, ref ) {
const { undo } = useDispatch( editorStore );
return (
<Button
// TODO: Switch to `true` (40px size) if possible
__next40pxDefaultSize={ false }
__next40pxDefaultSize
{ ...props }
ref={ ref }
icon={ ! isRTL() ? undoIcon : redoIcon }
Expand Down
3 changes: 1 addition & 2 deletions packages/editor/src/components/error-boundary/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,7 @@ function CopyButton( { text, children } ) {
const ref = useCopyToClipboard( text );
return (
<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
Error boundary before Error Boundary After

variant="secondary"
ref={ ref }
>
AKSHAT2802 marked this conversation as resolved.
Show resolved Hide resolved
Expand Down
3 changes: 1 addition & 2 deletions packages/editor/src/components/post-excerpt/panel.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,8 +198,7 @@ function PrivateExcerpt() {
ref={ setPopoverAnchor }
renderToggle={ ( { onToggle } ) => (
<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-excerpt before Post excerpt after

The style didn't change because there are explicit styles for Add and excerpt in code. But adding __next40pxDefaultSize will ensure that it will not be listed in lint next time

className="editor-post-excerpt__dropdown__trigger"
AKSHAT2802 marked this conversation as resolved.
Show resolved Hide resolved
onClick={ onToggle }
variant="link"
Expand Down
9 changes: 3 additions & 6 deletions packages/editor/src/components/post-featured-image/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,8 +160,7 @@ function PostFeaturedImage( {
render={ ( { open } ) => (
<div className="editor-post-featured-image__container">
<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
Featured Image before Featured image after

Copy link
Member

Choose a reason for hiding this comment

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

Looks like we can remove most of these style overrides?

gutenberg/packages/editor/src/components/post-featured-image/style.scss

Lines 67 to 73 in 9523fb4

.editor-post-featured-image__toggle {
height: 100%;
line-height: 20px;
padding: $grid-unit-10 0;
text-align: center;
box-shadow: inset 0 0 0 $border-width $gray-400;
}

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
Screenshot 2024-09-12 at 1 10 28 AM Screenshot 2024-09-12 at 1 11 21 AM

Hii @mirka removed extra styles
PFA Before and After screenshots

ref={ toggleRef }
className={
! featuredImageId
Expand Down Expand Up @@ -202,17 +201,15 @@ function PostFeaturedImage( {
{ !! featuredImageId && (
<HStack className="editor-post-featured-image__actions">
<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
replace-remove before replace remove after

Copy link
Member

Choose a reason for hiding this comment

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

Looks like the before/after images are reversed in this one.

We probably need an !important here:

gutenberg/packages/editor/src/components/post-featured-image/style.scss

Line 56 in 9523fb4

height: auto;

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
Screenshot 2024-09-12 at 12 57 25 AM Screenshot 2024-09-12 at 12 55 49 AM

Hii @mirka , height: auto !important; added for featured image.
PFA new screenshots.

Copy link
Member

Choose a reason for hiding this comment

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

I noticed a weird flash on focus, but this happens on trunk too so I'll post a separate issue.

CleanShot.2024-09-13.at.05.11.47.mp4

Copy link
Member

Choose a reason for hiding this comment

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

Issue submitted at #65299

className="editor-post-featured-image__action"
onClick={ open }
aria-haspopup="dialog"
>
{ __( 'Replace' ) }
</Button>
<Button
// TODO: Switch to `true` (40px size) if possible
__next40pxDefaultSize={ false }
__next40pxDefaultSize
className="editor-post-featured-image__action"
onClick={ () => {
onRemoveImage();
Expand Down
Loading