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

Editor: Cleanup styles and classnames #62237

Merged
merged 3 commits into from
Jun 4, 2024
Merged

Editor: Cleanup styles and classnames #62237

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
cbd6318
Editor: Cleanup styles and classnames
youknowriad Jun 3, 2024
075668e
Remove useless div
youknowriad Jun 3, 2024
55dbce1
Fix build
youknowriad Jun 3, 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
6 changes: 2 additions & 4 deletions packages/base-styles/_z-index.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ $z-layers: (
".block-editor-warning": 5,
".block-library-gallery-item__inline-menu": 20,
".block-editor-url-input__suggestions": 30,
".edit-post-layout__footer": 30,
".interface-interface-skeleton__header": 30,
".interface-interface-skeleton__content": 20,
".edit-widgets-header": 30,
Expand Down Expand Up @@ -86,13 +85,12 @@ $z-layers: (
// Show sidebar above wp-admin navigation bar for mobile viewports:
// #wpadminbar { z-index: 99999 }
".interface-interface-skeleton__sidebar": 100000,
".edit-post-layout__toggle-sidebar-panel": 100000,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In my test this classname was not being used.

".editor-layout__toggle-sidebar-panel": 100000,
".edit-widgets-sidebar": 100000,
".edit-post-layout .edit-post-post-publish-panel": 100001,
".editor-post-publish-panel": 100001,
// For larger views, the wp-admin navbar dropdown should be at top of
// the Publish Post sidebar.
".edit-post-layout .edit-post-post-publish-panel {greater than small}": 99998,
".editor-post-publish-panel {greater than small}": 99998,

// For larger views, the wp-admin navbar dropdown should be on top of
// the multi-entity saving sidebar.
Expand Down
4 changes: 0 additions & 4 deletions packages/block-library/src/navigation/editor.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -468,10 +468,6 @@ $color-control-label-height: 20px;
top: $admin-bar-height + $header-height + $block-toolbar-height + $border-width;
}

.is-sidebar-opened .wp-block-navigation__responsive-container.is-menu-open {
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-sidebar-opened classname was not being added in the site editor (where the navigation block is mostly used) which means that I just removed as this style is probably not useful.

right: $sidebar-width;
}

// When fullscreen.
.is-fullscreen-mode {
.wp-block-navigation__responsive-container.is-menu-open {
Expand Down
9 changes: 0 additions & 9 deletions packages/edit-post/src/components/layout/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ import {
privateApis as blockEditorPrivateApis,
store as blockEditorStore,
} from '@wordpress/block-editor';
import { useViewportMatch } from '@wordpress/compose';
import { PluginArea } from '@wordpress/plugins';
import { __, sprintf } from '@wordpress/i18n';
import { useCallback, useMemo } from '@wordpress/element';
Expand Down Expand Up @@ -144,19 +143,16 @@ function useEditorStyles() {
function Layout( { initialPost } ) {
useCommands();
useEditPostCommands();
const isWideViewport = useViewportMatch( 'large' );
const paddingAppenderRef = usePaddingAppender();
const shouldIframe = useShouldIframe();
const { createErrorNotice } = useDispatch( noticesStore );
const {
mode,
isFullscreenActive,
sidebarIsOpened,
hasActiveMetaboxes,
hasBlockSelected,
showIconLabels,
isDistractionFree,
showBlockBreadcrumbs,
showMetaBoxes,
hasHistory,
isEditingTemplate,
Expand All @@ -175,7 +171,6 @@ function Layout( { initialPost } ) {
!! select( blockEditorStore ).getBlockSelectionStart(),
showIconLabels: get( 'core', 'showIconLabels' ),
isDistractionFree: get( 'core', 'distractionFree' ),
showBlockBreadcrumbs: get( 'core', 'showBlockBreadcrumbs' ),
showMetaBoxes:
select( editorStore ).getRenderingMode() === 'post-only',
hasHistory: !! getEditorSettings().onNavigateToPreviousEntityRecord,
Expand All @@ -201,11 +196,7 @@ function Layout( { initialPost } ) {
}

const className = clsx( 'edit-post-layout', 'is-mode-' + mode, {
'is-sidebar-opened': sidebarIsOpened,
'has-metaboxes': hasActiveMetaboxes,
'is-distraction-free': isDistractionFree && isWideViewport,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved this class within the component.

'has-block-breadcrumbs':
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 was not used.

Copy link
Member

Choose a reason for hiding this comment

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

We need to keep the has-block-breadcrumbs class to avoid a regression for this one

showBlockBreadcrumbs && ! isDistractionFree && isWideViewport,
} );

function onPluginAreaError( name ) {
Expand Down
2 changes: 1 addition & 1 deletion packages/edit-site/src/components/editor/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,7 +234,7 @@ export default function Editor( { isLoading } ) {
<SiteEditorMoreMenu />
<EditorInterface
className={ clsx(
'edit-site-editor__interface-skeleton',
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like WooCommerce uses this class?
https://wpdirectory.net/search/01HZF50T58QDM6WW7MP6WHQ7Y1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They are actually applying the class them selves, so it should be fine

https://github.com/woocommerce/woocommerce/blob/f7ef2ca91b79d17fe3ac821723a9b97e4cc680b5/plugins/woocommerce-admin/client/customize-store/assembler-hub/editor.tsx#L78

They probably don't need to apply it at all though, it seems useless looking at their code base.

'edit-site-editor__editor-interface',
{
'show-icon-labels': showIconLabels,
}
Expand Down
2 changes: 1 addition & 1 deletion packages/edit-site/src/components/editor/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
.edit-site-editor__interface-skeleton {
.edit-site-editor__editor-interface {
opacity: 1;
transition: opacity 0.1s ease-out;
@include reduce-motion("transition");
Expand Down
9 changes: 0 additions & 9 deletions packages/edit-site/src/components/layout/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,6 @@ export default function Layout() {
canvasMode,
previousShortcut,
nextShortcut,
hasBlockBreadcrumbs,
} = useSelect( ( select ) => {
const { getAllShortcutKeyCombinations } = select(
keyboardShortcutsStore
Expand All @@ -102,10 +101,6 @@ export default function Layout() {
'core',
'distractionFree'
),
hasBlockBreadcrumbs: select( preferencesStore ).get(
'core',
'showBlockBreadcrumbs'
),
hasBlockSelected:
select( blockEditorStore ).getBlockSelectionStart(),
};
Expand Down Expand Up @@ -185,10 +180,6 @@ export default function Layout() {
'is-full-canvas': canvasMode === 'edit',
'has-fixed-toolbar': hasFixedToolbar,
'is-block-toolbar-visible': hasBlockSelected,
'has-block-breadcrumbs':
Copy link
Member

Choose a reason for hiding this comment

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

We need to keep the has-block-breadcrumbs class to avoid a regression for this one

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We've discussed recently with @jameskoster that the snackbars should remain in the exact same position regardless of what the UI is showing. It's already the case in trunk and this class is not used at all in our styles.

Copy link
Member

Choose a reason for hiding this comment

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

Thank you. Is there any place I can see the discussion around the topic?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, in this PR #61676

Copy link
Member

Choose a reason for hiding this comment

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

Thank you!

hasBlockBreadcrumbs &&
! isDistractionFree &&
canvasMode === 'edit',
}
) }
>
Expand Down
8 changes: 4 additions & 4 deletions packages/editor/src/components/editor-interface/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,8 +121,10 @@ export default function EditorInterface( {
<InterfaceSkeleton
enableRegionNavigation={ enableRegionNavigation }
isDistractionFree={ isDistractionFree && isWideViewport }
className={ clsx( className, {
className={ clsx( 'editor-editor-interface', className, {
'is-entity-save-view-open': !! entitiesSavedStatesCallback,
'is-distraction-free':
isDistractionFree && isWideViewport && ! isPreviewMode,
} ) }
labels={ {
...interfaceLabels,
Expand Down Expand Up @@ -206,9 +208,7 @@ export default function EditorInterface( {
isRichEditingEnabled &&
blockEditorMode !== 'zoom-out' &&
mode === 'visual' && (
<div className="edit-post-layout__footer">
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 this div as it was useless in my tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

What does "it was useless" mean? https://wpdirectory.net/search/01HZF570S4FPCYK4W9343A277D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was useless within Gutenberg.

Looking at the link above, it seems all of the plugins are actually rendering this div and targeting it themselves. They're basically copy/pasting Gutenberg code in their plugins I'd say. So this change won't have any impact on them.

The only one that can be slightly impacted I think is "Virtual Classroom – Video Conferencing & Online Meeting with BigBlueButton". They're hiding the div using CSS.

<BlockBreadcrumb rootLabelText={ documentLabel } />
</div>
<BlockBreadcrumb rootLabelText={ documentLabel } />
)
}
actions={
Expand Down
3 changes: 3 additions & 0 deletions packages/editor/src/components/editor-interface/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
.editor-editor-interface .entities-saved-states__panel-header {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Move this style from and applied a style that doesn't rely on external classnames.

height: $header-height + $border-width;
}
7 changes: 0 additions & 7 deletions packages/editor/src/components/entities-saved-states/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,13 +31,6 @@
}
}

.edit-post-layout,
.edit-site-editor__interface-skeleton {
.entities-saved-states__panel-header {
height: $header-height + $border-width;
}
}

.entities-saved-states__post-meta {
margin-left: $grid-unit-30;
align-items: center;
Expand Down
2 changes: 1 addition & 1 deletion packages/editor/src/components/header/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,7 +190,7 @@
}
}

.is-distraction-free {
.editor-editor-interface.is-distraction-free {
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 doesn't change anything really but it makes things easier to understand. Relying on global utility classnames is not great because we don't really know who adds the class, the impact...

.interface-interface-skeleton__header {
border-bottom: none;
}
Expand Down
63 changes: 27 additions & 36 deletions packages/editor/src/components/post-publish-panel/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,3 @@
.editor-post-publish-panel {
background: $white;
}

.editor-post-publish-panel__content {
// Ensure the post-publish panel accounts for the header and footer height.
min-height: calc(100% - #{$header-height + 84px});
Expand Down Expand Up @@ -71,6 +67,7 @@

.editor-post-publish-panel__header-publish-button {
padding-right: $grid-unit-05;
justify-content: center;
}

.editor-post-publish-panel__header-cancel-button {
Expand Down Expand Up @@ -194,40 +191,34 @@
}
}

.edit-post-layout,
.edit-site-editor__interface-skeleton {
.editor-post-publish-panel {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was not clear to me why these wrappers were needed, so I just removed them and the publish panel sidebars look the same for me. Am I missing something @ntsekouras

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't remember exactly, but I think it was something about the top property for sure and was oriented towards bottom. I tested now and it seemed fine..

Copy link
Contributor

Choose a reason for hiding this comment

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

Hey @youknowriad & @ntsekouras 👋🏼 Turns out there's a small regression. I have created a PR, but don't have write permissions yet: #62736

Feel free to take a look. Thank you. 🙂

position: fixed;
z-index: z-index(".edit-post-layout .edit-post-post-publish-panel");
top: $admin-bar-height-big;
bottom: 0;
right: 0;
left: 0;
overflow: auto;

@include break-medium() {
z-index: z-index(".edit-post-layout .edit-post-post-publish-panel {greater than small}");
top: $admin-bar-height;
left: auto;
width: $sidebar-width + $border-width;
border-left: $border-width solid $gray-300;
transform: translateX(+100%);
animation: editor-post-publish-panel__slide-in-animation 0.1s forwards;
@include reduce-motion("animation");

body.is-fullscreen-mode & {
top: 0;
}

// Keep it open on focus to avoid conflict with navigate-regions animation.
[role="region"]:focus & {
transform: translateX(0%);
}
.editor-post-publish-panel {
position: fixed;
z-index: z-index(".editor-post-publish-panel");
background: $white;
top: 0;
bottom: 0;
right: 0;
left: 0;
overflow: auto;

@include break-medium() {
z-index: z-index(".editor-post-publish-panel {greater than small}");
top: $admin-bar-height;
left: auto;
width: $sidebar-width + $border-width;
border-left: $border-width solid $gray-300;
transform: translateX(+100%);
animation: editor-post-publish-panel__slide-in-animation 0.1s forwards;
@include reduce-motion("animation");

body.is-fullscreen-mode & {
top: 0;
}
}

.editor-post-publish-panel__header-publish-button {
justify-content: center;
// Keep it open on focus to avoid conflict with navigate-regions animation.
[role="region"]:focus & {
transform: translateX(0%);
}
}
}

Expand Down
8 changes: 0 additions & 8 deletions packages/editor/src/components/save-publish-panels/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,11 +26,3 @@
bottom: 0;
}
}

.edit-post-layout__toggle-sidebar-panel {
.interface-interface-skeleton__sidebar:focus &,
.interface-interface-skeleton__sidebar:focus-within & {
top: auto;
bottom: 0;
}
}
1 change: 1 addition & 0 deletions packages/editor/src/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
@import "./components/document-bar/style.scss";
@import "./components/document-outline/style.scss";
@import "./components/document-tools/style.scss";
@import "./components/editor-interface/style.scss";
@import "./components/editor-notices/style.scss";
@import "./components/entities-saved-states/style.scss";
@import "./components/error-boundary/style.scss";
Expand Down
17 changes: 0 additions & 17 deletions packages/interface/src/components/interface-skeleton/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,13 +114,6 @@ html.interface-interface-skeleton__html-container {
// On Mobile the header is fixed to keep HTML as scrollable.
@include break-medium() {
position: relative !important;

// Set this z-index only when the sidebar is opened. When it's closed, the
// button to open the sidebar that is shown when navigating regions needs to
// be above the footer. See `editor-layout__toggle-sidebar-panel`.
.is-sidebar-opened & {
z-index: z-index(".interface-interface-skeleton__sidebar {greater than small}");
}
}
}

Expand Down Expand Up @@ -170,16 +163,6 @@ html.interface-interface-skeleton__html-container {
@include break-medium() {
display: flex;
}

.block-editor-block-breadcrumb {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In my test the breadcrumb looked the same with and without this so I think this was useless (and misplaced anyway)

Copy link
Contributor

Choose a reason for hiding this comment

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

Current usage: https://wpdirectory.net/search/01HZF59YSMNJB2CYSYYC08CK8Q

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 class hasn't been removed. It's the style that was removed.

Copy link
Contributor

Choose a reason for hiding this comment

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

@youknowriad it looks like this removal has caused a regression in trunk in the post and site editors when no block is selected:

image

As far as I can tell the display: flex, height and align-items were necessary here, and the left padding helped keep the breadcrumbs from bumping up against the edge of the window.

I've put up a revert PR over in #62309 for just this bit of styling to get it back to what it was. We could then look at moving the CSS elsewhere in a follow-up if there's a better place for it?

z-index: z-index(".edit-post-layout__footer");
display: flex;
background: $white;
height: $button-size-small;
align-items: center;
font-size: $default-font-size;
padding: 0 ($grid-unit-15 + 6px);
}
}

.interface-interface-skeleton__actions {
Expand Down
Loading