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

Don't overlap canvas with inserter panel at large screens #64110

Merged
merged 14 commits into from
Jul 31, 2024
Merged

Don't overlap canvas with inserter panel at large screens #64110

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
e831544
Don't overlap canvas with inserter panel at large screens
jeryj Jul 30, 2024
e433b16
Always resize canvas
jeryj Jul 30, 2024
aa4fd01
Cleanup
jeryj Jul 30, 2024
d250bc8
Refactor to share new -sidebar-width var
jeryj Jul 30, 2024
be2dbe7
Try to improve animation -- still needs work
jeryj Jul 30, 2024
c95391e
animate the pattern category preview panel
scruffian Jul 31, 2024
72c808a
use the same animation for both elements
scruffian Jul 31, 2024
ae37927
Add animate presence
scruffian Jul 31, 2024
bbf1b2e
only use one animated div
scruffian Jul 31, 2024
29314f8
Pattern panel zindex
jeryj Jul 31, 2024
f6a84af
Animate pattern tab panel on open
jeryj Jul 31, 2024
86b42a2
Try to smooth entrance and exit secondary sidebar animations
jeryj Jul 31, 2024
2525a5f
remove unneeded properties so that the parent controls the animation
scruffian Jul 31, 2024
f764e96
remove unneeded double shadow
richtabor Jul 31, 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
2 changes: 1 addition & 1 deletion packages/base-styles/_variables.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ $sidebar-width: 280px;
$content-width: 840px;
$wide-content-width: 1100px;
$widget-area-width: 700px;

$secondary-sidebar-width: 350px;

/**
* Block & Editor UI.
Expand Down
37 changes: 35 additions & 2 deletions packages/block-editor/src/components/inserter/category-tabs/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,13 @@
/**
* WordPress dependencies
*/
import { usePrevious, useReducedMotion } from '@wordpress/compose';
import { isRTL } from '@wordpress/i18n';
import {
__experimentalHStack as HStack,
FlexBlock,
privateApis as componentsPrivateApis,
__unstableMotion as motion,
} from '@wordpress/components';
import { Icon, chevronRight, chevronLeft } from '@wordpress/icons';

Expand All @@ -22,6 +24,17 @@ function CategoryTabs( {
onSelectCategory,
children,
} ) {
// Copied from InterfaceSkeleton.
const ANIMATION_DURATION = 0.25;
const disableMotion = useReducedMotion();
const defaultTransition = {
type: 'tween',
duration: disableMotion ? 0 : ANIMATION_DURATION,
ease: [ 0.6, 0, 0.4, 1 ],
};

const previousSelectedCategory = usePrevious( selectedCategory );

return (
<Tabs
className="block-editor-inserter__category-tabs"
Expand Down Expand Up @@ -62,9 +75,29 @@ function CategoryTabs( {
key={ category.name }
tabId={ category.name }
focusable={ false }
className="block-editor-inserter__category-panel"
>
{ children }
<motion.div
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it better to have the motion div inside each tab, or could we put this outside categories.map?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The tab won't mount its content until it is selected, so this is essentially one div until it mounts. It's possible it's better if we have it outside of it, but I'm concerned with that method since until we have a tabpanel selected, the semantics are off. I think this way with the motion div inside is safer semantically.

Copy link
Contributor

Choose a reason for hiding this comment

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

The motion div only gets mounted when the category is mounted, so we don't generate loads of motion divs with this approach anyway.

className="block-editor-inserter__category-panel"
initial={
! previousSelectedCategory ? 'closed' : 'open'
}
animate="open"
variants={ {
open: {
transform: 'translateX( 0 )',
transitionEnd: {
zIndex: '1',
},
},
closed: {
transform: 'translateX( -100% )',
zIndex: '-1',
},
} }
transition={ defaultTransition }
>
{ children }
</motion.div>
</Tabs.TabPanel>
) ) }
</Tabs>
Expand Down
27 changes: 16 additions & 11 deletions packages/block-editor/src/components/inserter/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
$block-inserter-preview-height: 350px;
$block-inserter-width: 350px;
$block-quick-inserter-width: 350px;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renaming this as I kept getting confused since this variable was only applicable to the quick inserter (the inline inserter in the canvas)

Copy link
Member

Choose a reason for hiding this comment

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

Worth an inline comment as well, defining what that is (for future reference?)

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
$block-quick-inserter-width: 350px;
$block-quick-inserter-width: 350px; // Only used in the quick inserter.

$block-inserter-tabs-height: 44px;

.block-editor-inserter {
Expand All @@ -24,6 +24,12 @@ $block-inserter-tabs-height: 44px;
&.show-as-tabs {
gap: 0;
}

.block-editor-tabbed-sidebar {
@include break-medium() {
width: $secondary-sidebar-width;
}
}
}

.block-editor-inserter__popover.is-quick {
Expand Down Expand Up @@ -85,6 +91,12 @@ $block-inserter-tabs-height: 44px;
height: 100%;
position: relative;
overflow: visible;

&.show-panel {
@include break-medium() {
width: $secondary-sidebar-width + $sidebar-width;
}
}
}

.block-editor-inserter__inline-elements {
Expand Down Expand Up @@ -299,14 +311,13 @@ $block-inserter-tabs-height: 44px;
@include break-medium {
border-left: $border-width solid $gray-200;
padding: 0;
left: 100%;
width: 300px;
left: $secondary-sidebar-width;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is now a fixed number, but we need it to be to know how far to offset. The category panel content still needs to be absolutely positioned as it's within the flow of the tabs.

width: $sidebar-width;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@richtabor This was previously fixed at 300px and updating this to $sidebar-width makes it thinner at 280px. I think it still looks good but wanted to flag this.

Copy link
Member

Choose a reason for hiding this comment

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

Yup, it's good to go.

position: absolute;
top: -$border-width;
height: calc(100% + #{$border-width});
background: $gray-100;
border-top: $border-width solid $gray-200;
box-shadow: $border-width $border-width 0 0 rgba($color: #000, $alpha: 0.133); // 0.133 = $gray-200 but with alpha.

.block-editor-inserter__media-list,
.block-editor-block-patterns-list {
Expand Down Expand Up @@ -366,7 +377,7 @@ $block-inserter-tabs-height: 44px;
max-width: 100%;

@include break-medium {
width: $block-inserter-width;
width: $block-quick-inserter-width;
}
}

Expand Down Expand Up @@ -679,12 +690,6 @@ $block-inserter-tabs-height: 44px;
height: 100%;
}
}

// Remove doubled-up shadow that occurs when categories panel is opened, only in zoom out.
// Shadow cannot be removed from the source, as it is required when not zoomed out.
.block-editor-inserter__category-panel {
box-shadow: none;
}
}

.show-icon-labels {
Expand Down
5 changes: 1 addition & 4 deletions packages/block-editor/src/components/tabbed-sidebar/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,10 @@
.block-editor-tabbed-sidebar {
background-color: $white;
height: 100%;
display: flex;
flex-direction: column;
flex-grow: 1;
overflow: hidden;

@include break-medium() {
width: 350px;
}
Comment on lines -7 to -10
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This shouldn't have been fixed at 350, as it shouldn't care what the parent width is. This would need to be removed to use it within the right side setting sidebar anyways.

}

.block-editor-tabbed-sidebar__tablist-and-close-button {
Expand Down
4 changes: 4 additions & 0 deletions packages/editor/src/components/list-view-sidebar/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
.editor-list-view-sidebar {
height: 100%;

@include break-medium {
width: $secondary-sidebar-width;
}
}

.editor-list-view-sidebar__list-view-panel-content,
Expand Down
16 changes: 9 additions & 7 deletions packages/interface/src/components/interface-skeleton/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,30 +182,32 @@ function InterfaceSkeleton(
ariaLabel={ mergedLabels.secondarySidebar }
as={ motion.div }
initial="closed"
animate={
isMobileViewport ? 'mobileOpen' : 'open'
}
animate="open"
exit="closed"
variants={ {
open: { width: secondarySidebarSize.width },
closed: { width: 0 },
mobileOpen: { width: '100vw' },
} }
transition={ defaultTransition }
>
<div
<motion.div
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need two motion components?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One sets the width of the sidebar, and one allows the content to slide in on top of it. We need the sidebar to use a width so it can push the canvas over smoothly. The X value didn't work for that. And if we only use width, then then content of the sidebar reflows instead of being a fixed width.

Copy link
Contributor

Choose a reason for hiding this comment

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

Because the parent animation is a width, not a transform, the child doesn't move with it, as it is absolutely positioned. We did try using a transform for the parent but that broke the animations for the canvas, because the canvas resizes based on the width of the sidebar.

style={ {
position: 'absolute',
width: isMobileViewport
? '100vw'
: 'fit-content',
height: '100%',
right: 0,
left: 0,
} }
variants={ {
open: { x: 0 },
closed: { x: '-100%' },
} }
transition={ defaultTransition }
>
{ secondarySidebarResizeListener }
{ secondarySidebar }
</div>
</motion.div>
</NavigableRegion>
) }
</AnimatePresence>
Expand Down
Loading