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

Decouple ComplementaryArea and Panel #40720

Open
ciampo opened this issue Apr 29, 2022 · 6 comments
Open

Decouple ComplementaryArea and Panel #40720

ciampo opened this issue Apr 29, 2022 · 6 comments
Labels
[Feature] Component System WordPress component system [Package] Components /packages/components [Package] Interface /packages/interface [Type] Code Quality Issues or PRs that relate to code quality [Type] Discussion For issues that are high-level and not yet ready to implement.

Comments

@ciampo
Copy link
Contributor

ciampo commented Apr 29, 2022

What problem does this address?

Initially flagged in #38934 (comment)

Currently, the ComplementaryArea and ComplementaryAreaHeader components from @wordpress/interface use the Panel component from @wordpress/component in an incorrect way:

  • the ComplementaryAreaHeader component uses the components-panel__header a few times to "borrow" the styles of the PanelHeader component — this classname should be considered private and shouldn't be used this way.
  • the ComplementaryAreaHeader also overrides styles associated to the components-panel__header classname — this is also an anti-patter that should be avoided. Internal component classnames should not be considered part of a component's public API, and therefore should be used outside of the component itself.
  • the ComplementaryArea component renders a Panel as a sibling of ComplementaryAreaHeader. This may need further investigation, in order to understand what were the original intentions here.
  • the ComplementaryArea component also heavily overriders the Panel styles by using its private hardcoded classnames, completely defeating the purpose of using a component.

What is your proposed solution?

I think that we should first understand what is the goal of ComplementaryArea (the way it's supposed to work AND look). Currently, the component uses Panel in such hacky ways that it makes me wonder if the Panel component is a good fit for ComplementaryArea at all — a few questions that might help with this investigation are:

  • Is there an actual need for ComplementaryAreaHeader to render a PanelHeader component ? And consequently,
  • Is there an actual need for ComplementaryArea to render a Panel as a sibling of a (de facto) PanelHeader ?

Based on this investigation, I see two main alternative approaches:

Option A: Remove Panel from ComplementaryArea, find an alternative

In case we agree that Panel is not a good fit for ComplementaryArea, we could remove it completely (including all of the hardcoded classnames), and look at reimplementing those parts:

  • either by considering alternative components (if any);
  • or by implementing those parts of the UI directly in the ComplementaryArea.

Option B: Continue using Panel, but in a tidier, proper way

This would be the option in case we think that Panel can fit ComplementaryArea's needs. The next steps, in this case, would be:

  1. Assess if there's any tweaks to be made in the Panel component (that don't introduce breaking changes) that can benefit ComplementaryArea
  2. Look into using the Panel and PanelHeader components in ComplementaryArea and ComplementaryAreaHeader, instead of re-using the hardcoded components-panel__header classname
  3. Look into minimising the style overrides for the Panel components as much as possible. These overrides should be applied without using hardcoded classnames from the Panel component.

Regardless of which option we pick, we should finally add ComplementaryArea (and its subcomponents) to Storybook.

@ciampo ciampo added [Type] Code Quality Issues or PRs that relate to code quality [Package] Components /packages/components [Package] Interface /packages/interface [Type] Discussion For issues that are high-level and not yet ready to implement. [Feature] Component System WordPress component system labels Apr 29, 2022
@ciampo
Copy link
Contributor Author

ciampo commented Apr 29, 2022

@mirka how does the plan above sound to you?

@mirka
Copy link
Member

mirka commented Apr 29, 2022

Sounds good. This maybe obvious, but the only thing I'd add is that we'll want to add ComplementaryArea and friends to Storybook while we're at it.

@ciampo
Copy link
Contributor Author

ciampo commented Apr 29, 2022

Not obvious at all! That would be the first instance of a component from @wordpress/interface too 😄

Added to the original issue description 👌

@talldan
Copy link
Contributor

talldan commented May 2, 2022

My understanding is that any of the sidebars on the right of the editor are complementary areas.

So that's:

  • Document settings / Block inspector in most of the editors
  • The site editor's styles panel
  • The site editor's navigation panel
  • Plugin areas (the post editor is probably the best example of this)

I wasn't involved in the development, but I could take a couple of guesses at how this came about. For a start, Complementary Areas were definitely refactored out of the edit-post/editor package which originally had plugin areas. I'd imagine this misuse of components was there from the very early days of Gutenberg. And I think it was probably like that because the Panel components are quite opinionated and aren't very flexible.

For example: only the document settings and block inspector components use PanelRow components. The other Panels are just headers with empty canvases. The latter use case is something that seems to be happening more and more, but the components haven't been updated to handle that—the description in the README for Panel says "Panels expand and collapse multiple sections of content".

@ciampo
Copy link
Contributor Author

ciampo commented May 2, 2022

I'd imagine this misuse of components was there from the very early days of Gutenberg

Thank you @talldan for the insights — this was also our guess.

[...] the Panel components are quite opinionated and aren't very flexible.

For example: only the document settings and block inspector components use PanelRow components. The other Panels are just headers with empty canvases. The latter use case is something that seems to be happening more and more, but the components haven't been updated to handle that—the description in the README for Panel says "Panels expand and collapse multiple sections of content".

The Panel component definitely seems a bit out of date and not very flexible, and that may be the primary reason why we see these "hacky" ways around it — although I would also argue that the best way to deal with this king of limitations would have been to improve the original component instead.

It would be great if someone with a good understanding of Gutenberg's UI could take a close look at the Panel component, and:

  • understand if it's still a relevant component in today's Gutenberg
  • find all of its usages and coming up with a list of all its limitations as of today (e.g. the ones you just mentioned above)

We could then understand what improvements can be made to the original Panel component to overcome these limitations (without introducing breaking changes), and apply these changes throughout the codebase.

@talldan
Copy link
Contributor

talldan commented May 3, 2022

understand if it's still a relevant component in today's Gutenberg

Yes, it's used extensively (and correctly afaik) in the BlockInspector and the Document settings in the post editor. Also used by lots of plugins.

find all of its usages and coming up with a list of all its limitations as of today (e.g. the ones you just mentioned above)

I think what I mentioned is pretty much the extent of the usage. The good thing is it would be relatively easy to see the outcome of any changes to ComplementaryArea. It's probably worth starting by trying to remove the broken styles and seeing what breaks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Component System WordPress component system [Package] Components /packages/components [Package] Interface /packages/interface [Type] Code Quality Issues or PRs that relate to code quality [Type] Discussion For issues that are high-level and not yet ready to implement.
Projects
None yet
Development

No branches or pull requests

3 participants