WordPress / gutenberg

The Block Editor project for WordPress and beyond. Plugin is available from the official repository.
https://wordpress.org/gutenberg/
Other
10.52k stars 4.2k forks source link

Merge `devicePreviewType` state within a single store #37759

Open zaguiini opened 2 years ago

zaguiini commented 2 years ago

What problem does this address?

Right now, we have the exact same logic for getDevicePreviewType and setDevicePreviewType inside the core/edit-post and core/edit-site.

What is your proposed solution?

As that information is used in both kinds of editor, it makes sense to unify it into a single source of truth -- the core/block-editor store.

It already caused a bug and it might cause others in the future.

The idea is to expose both the action and selector inside core/block-editor so those inconsistencies go away and we can stop doing nasty workarounds.

Tasks

We can only deploy 2 once Gutenberg is published. We can only deploy 3 once the new version of block-experiments is published.

youknowriad commented 2 years ago

Hi there! thanks for opening the issue, For me, it doesn't make sense to be in "core/block-editor". The block editor package is about building reusable block editors (think of it as TinyMCE), meaning you could have multiple in a single page... While the device preview is more of a "editor screen" option.

I get the "Don't repeat yourself principle" but sometimes this causes more harm than good and in this situations, I'm not really sure it's worth sharing code.

zaguiini commented 2 years ago

For me, it doesn't make sense to be in "core/block-editor"

Honestly, it does not feel like the appropriate place. I share the same feeling.

Where could we put it, then?

but sometimes this causes more harm than good

I also agree with that...

I'm not really sure it's worth sharing code.

However, this looks like the right move in this case. As I said, it already caused bugs and who knows how many it could cause in the future 😐

I do think abstracting is a better option. Any thoughts on where to put it? I saw core/viewport but it also doesn't look ideal.

youknowriad commented 2 years ago

To be honest I don't know either 😬

I think what worked well in terms of reusing code in the past is extracting packages with a defined purpose and ideally stateless packages. (Dom, url, priority-queue...). The code we want to share here has two issues:

Mamaduka commented 2 years ago

Can we use @packages/interface for this? It already has a store and is used by all editor screens.

youknowriad commented 2 years ago

If I'm being honest I'd say that the current store of the interface package is also a mistake.

The interface package has been initially about providing shared UI elements to the screens (skeleton) but it grew weirdly that now it's very unclear what it serves aside being a place to share code without any meaningful purpose. My ideal scenario would be to restore the original purpose of interface but I can also accept that the ship has sailed there and that this package will be a weird one forever :P

zaguiini commented 2 years ago

Thanks, @youknowriad!

the purpose of an extracted package based on this code is not clear

I think it's clear enough 🤔 We need to standardize how we deal with preview breakpoints. Having two different implementations for the same thing in the post and the site editor is far from ideal.