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

is-sidebar-opened class missing and sidebar has fixed inline width #62599

Closed
amplify11dev opened this issue Jun 15, 2024 · 16 comments
Closed

is-sidebar-opened class missing and sidebar has fixed inline width #62599

amplify11dev opened this issue Jun 15, 2024 · 16 comments
Labels
[Package] Editor /packages/editor [Type] Bug An existing feature does not function as intended

Comments

@amplify11dev
Copy link

Description

Previously when opening the right sidebar (settings panel) a classname is-sidebar-opened was added to the dom. This allowed developers to know when that sidebar was opened and target the editor with styles. Right now it seems like the right sidebar has a fixed width of 280px (references to const SIDEBAR_WIDTH = 280; in the JS code). It's not clear how developers can adjust this width to be a % rather than a fixed width. The sidebar is far too narrow for our needs. In WordPress 6.5 and previous versions we were able to set the editor width to 60% and the sidebar to 40%. Then using the is-sidebar-opened class we could change the editor to 100% so there wasn't empty space. Without that classname, or ability to edit the width of the sidebar we're stuck with an empty 40% width div.

I'm not sure when or why this behavior changed, but I found this: https://github.com/WordPress/gutenberg/pull/62237/files related PR that seems like it might be removing the class name.

Step-by-step reproduction instructions

  • Add a new page

  • Open the right sidebar

  • Observe the class named is-sidebar-opened is visible in WordPress 6.5 and earlier

  • The width property was previously in CSS

  • Add a new page

  • Opened the right sidebar

  • Observe the class named is-sidebar-opened is gone.

  • The width property is now inline hard coded to the divs

Screenshots, screen recording, code snippet

WordPress 6.5 and earlier
Screenshot 2024-06-15 at 10 49 15 AM

WordPress 6.6 Beta
Screenshot 2024-06-15 at 10 50 19 AM

Environment info

Bug Report

Description

Describe the bug.

Environment

  • WordPress: 6.6-beta2
  • PHP: 8.1.23
  • Server: Apache/2.4.43 (Unix)
  • Database: mysqli (Server: 5.7.28 / Client: mysqlnd 8.1.23)
  • Browser: Chrome 125.0.0.0 (macOS)
  • Plugins:
    • Advanced Custom Fields PRO 6.3.1.2
    • Query Monitor 3.16.3
    • WordPress Beta Tester 3.5.5

Please confirm that you have searched existing issues in the repo.

Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

Yes

@amplify11dev amplify11dev added the [Type] Bug An existing feature does not function as intended label Jun 15, 2024
@jordesign jordesign added the [Package] Editor /packages/editor label Jun 17, 2024
@talldan
Copy link
Contributor

talldan commented Jun 18, 2024

@amplify11dev Class names like is-sidebar-opened are not part of a public API and so don't require backwards compatibility (this is documented here - https://github.com/WordPress/gutenberg/blob/trunk/docs/contributors/code/backward-compatibility.md#class-names-and-dom-updates).

I think in this case, there are alternative ways you can adjust the width, like using an !important style to override the inline style, and you can check the sidebar is opened by its presence in the DOM or using the preferences API (wp.data.select( 'core/preferences' ).get( 'core', 'isComplementaryAreaVisible' );)

@talldan talldan closed this as not planned Won't fix, can't repro, duplicate, stale Jun 18, 2024
@amplify11dev
Copy link
Author

The documentation also states that Changes to these should be done with caution as it can affect the styling and behavior of third-party code (Even if they should not rely on these in the first place). Keep the old ones if possible. If not, document the changes and write a dev note.. I don't see where this change is documented, discussed, or that caution was given when removing this. Furthermore I can't use !important as that would make the width of the div always visible and larger even when the sidebar should be closed. I'm trying to do this in styles, so the preferences API wouldn't be helpful either. If the classname won't be restored this should be re-opened and a solution should be implemented to allow developers to control the width of the sidebar. Hard coding the width into javascript isn't a best practice or good solution. This is a breaking change and regression from 6.5.

@talldan
Copy link
Contributor

talldan commented Jun 19, 2024

I'll cc @youknowriad who made the change.

This is a breaking change and regression from 6.5.

I did just share the docs that mention how this is not a breaking change, and it's certainly not a regression. Dev notes are still being worked on for 6.6, so I can follow up to make sure it's captured.

Have you considered making an issue requesting resizable sidebars as an enhancement? Alternatively an issue that proposes the editors to offer an API for setting the sidebar width?

@youknowriad
Copy link
Contributor

Sorry for the breakage @amplify11dev

As shared by @talldan, classnames are not part of the public API (especially modifiers like this) but sometimes we can keep them around if they help third-party developers.

In this case though, I'm personally hesitant to restore the classname, because it will add a re-rendering to the editor (all components) when the sidebar is changed causing a performance impact. So if we can find an alternative to what you're trying to do, we could document it in a dev note.

Definitely consider opening an issue about making the sidebar resizable or have a preference about it or something.

@amplify11dev
Copy link
Author

Thanks @youknowriad makes sense on the re-render consequence. Ultimately as a developer I just need to be able to control the width of the sidebar only when it's opened. Currently that's hard coded at 280px but we put a lot of stuff into that sidebar and it's easier for our content creators to have more room to work with in there. I'll open a new issue to make that width dynamic somehow. Perhaps that const width = 280 can become something we can filter in PHP or it uses a css3 variable for its width that we can change to a percentage or PX value.

@sergiu-radu
Copy link

Hi guys, we have the same issue with the sidebar.

We also where changing the width from CSS because 280px is not enough for our UI.

Maybe you could add a filter for changing that value?
We can even help with a PR but we have to know that this will be merged as soon as possible, maybe in 6.6.1 if not in 6.6, what do you think?

Please understand us right, this is a big problem for us at the moment, a lot of our clients will have a broken UI.

@stokesman
Copy link
Contributor

Furthermore I can't use !important as that would make the width of the div always visible and larger even when the sidebar should be closed.

We also where changing the width from CSS because 280px is not enough for our UI.

From my testing this can be worked around in CSS:

/* Apply the width only when the sidebar has children */
.interface-interface-skeleton__sidebar:not(:empty) {
    width: 33%;
}

.interface-complementary-area__fill,
.interface-complementary-area.editor-sidebar {
    width: 100% !important;
}

!important is needed on two elements but neither is actually present when the sidebar is closed so it doesn’t need to be conditionally applied.

Yes, the opening/closing won’t animate but that’s hardly critical.

@sergiu-radu
Copy link

Hi @stokesman, I also did some tests with CSS and found a solution but it is not very good.

  1. The animation is not working (like you already mentioned)
  2. There is a big delay when you close the sidebar (because the JS animation is running)

I think the best way would be to add a filter so developers could change that value from JS.
Let me know in case you want to see a PR, we will try to find some time and push it as soon as possible.

@stokesman
Copy link
Contributor

@sergiu-radu, you’re right the delay is not good. I'm fully in support of adding an API that developers could use partly because I think core could benefit from it too. At the moment, I’m not sure a filter is the best form it could take and tend to think something like extending enableComplementaryArea and setDefaultComplementaryArea to support a parameter that specifies the width could be better. That’s a just my initial idea and it might not be a good one. I’d be glad to see a PR though would expect much scrutiny and that it may only end up being a step toward figuring out what should be done here.

@albanyacademy
Copy link

personally i think its kinda weird an element is a parent of the block
image

@albanyacademy
Copy link

albanyacademy commented Jul 16, 2024

i'm curious - what's the performance advantage for animating width values for the block list + sidebar using framer vs just transitioning in/out? i can see framer potentially being more useful for the full site editor experience (though caveat im not super knowledgeable about the general performance differences between RAF and regular transitions for stuff like this - my understanding pretty much ends at "you can do certain things better with one or the other"), but if it was just a css transition you could provide the sidebar final width as a css variable

@stokesman
Copy link
Contributor

stokesman commented Jul 16, 2024

i think its kinda weird an element is a parent of the block

Me too. At least the class name is violating BEM. The __fill suffix should indicate that it’s a child of interface-complementary-area. I believe I have a branch where I happened to remove the element with no drawbacks.

what's the performance advantage for animating width values for the block list + sidebar using framer vs just transitioning in/out?

I don’t think that performance was a consideration. The sidebar (editor-sidebar) is unmounted/removed when its closed and framer-motion facilitates keeping it present/visible during the transition. It could be done another way but performance differences are likely to be negligible as long as properties like width are animated as they can’t be handled by the compositor.

@andreiglingeanu
Copy link
Contributor

Doing this whole animation with transitions is a much better option than trying to make the width configurable through the enableComplementaryArea / setDefaultComplementaryArea actions. Problems I see:

  1. Do we have to also make the mobileOpen state configurable? I think just making the desktop width configurable is enough but who knows?
  2. We have to expose the width prop all the way up to the PluginSidebarMoreMenuItem component. This is the entry point for the complementary areas defined by plugins

I'm happy to work on a PR for extending the above (and already started) but I really think the CSS transitions option + a CSS variable is an option worth exploring.

Thoughts?

@youknowriad
Copy link
Contributor

I'm happy to work on a PR for extending the above (and already started) but I really think the CSS transitions option + a CSS variable is an option worth exploring.

AFAIK, it's not possible to implement that way. If you manage to do it, please go ahead and open a PR.

For the width, I think we could have a preference, something like select('core/preferences').get( 'core', 'editorSidebarWidth' )

@albanyacademy
Copy link

what isn't possible, specifically? width can be transitioned with a css variable value

@youknowriad
Copy link
Contributor

@albanyacademy There are different constraints related to how the components are rendered... I don't want to discourage anyone from trying but it's more complex than just animating the width.

  • We need to animate the width while keeping the right width for the content of the sidebar from the start to avoid reflows
  • We need take into consideration that sometimes we transition from one sidebar to another and we should avoid animating in this case.
  • There's an extensibility aspect that complicates things as well (these sidebars can be registered...)

Feel free to try it and I would support it if it works.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Editor /packages/editor [Type] Bug An existing feature does not function as intended
8 participants