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

Merge devicePreviewType state within a single store #37759

Open
3 tasks
zaguiini opened this issue Jan 6, 2022 · 6 comments
Open
3 tasks

Merge devicePreviewType state within a single store #37759

zaguiini opened this issue Jan 6, 2022 · 6 comments
Labels
Needs Technical Feedback Needs testing from a developer perspective. [Type] Enhancement A suggestion for improvement.

Comments

@zaguiini
Copy link
Contributor

zaguiini commented Jan 6, 2022

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

  • Introduce devicePreviewType in core/block-editor (1);
  • Use core/block-editor version of devicePreviewType in block-experiments (2);
  • Remove core/edit-post and core/edit-site versions of devicePreviewType (3).

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

@youknowriad
Copy link
Contributor

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
Copy link
Contributor Author

zaguiini commented Jan 6, 2022

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
Copy link
Contributor

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:

  • It's statefull (and more application state than generic state)
  • the purpose of an extracted package based on this code is not clear.
@Mamaduka
Copy link
Member

Mamaduka commented Jan 7, 2022

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

@karmatosed karmatosed added the Needs Technical Feedback Needs testing from a developer perspective. label Jan 7, 2022
@youknowriad
Copy link
Contributor

youknowriad commented Jan 7, 2022

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
Copy link
Contributor Author

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.

@jordesign jordesign added the [Type] Enhancement A suggestion for improvement. label Aug 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Technical Feedback Needs testing from a developer perspective. [Type] Enhancement A suggestion for improvement.
5 participants