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

[Tracking] Command Palette: Turn off DFM mode when a command not suitable for DFM mode is executed #53993

Closed
14 of 17 tasks
t-hamano opened this issue Aug 28, 2023 · 9 comments
Closed
14 of 17 tasks
Assignees
Labels
[Package] Commands /packages/commands [Type] Bug An existing feature does not function as intended

Comments

@t-hamano
Copy link
Contributor

t-hamano commented Aug 28, 2023

What problem does this address?

The problem was discovered from this comment

Many commands do not take into account whether or not you are currently in DFM mode. As a result, unintended commands may be executed in DFM mode, causing layout corruption, etc.

What is your proposed solution?

As far as I could find out, at least the following commands should not be displayed in DFM mode.

Update: It seems better to leave these commands in place and remove DFM mode when they are executed.

Post Editor

  • Toggle block inspector
  • Toggle settings inspector
  • Toggle list view
  • Toggle top toolbar
  • Toggle code editor / Exit code editor: This can be toggled from the options menu, but I think that it would be better to force visual editor mode in DFM mode and disable the switch to the code editor.
  • Hide block breadcrumbs / Show block breadcrumbs: The breadcrumb is not always displayed in DFM mode. Is this command necessary?

Site Editor

@t-hamano t-hamano added [Type] Bug An existing feature does not function as intended [Package] Commands /packages/commands labels Aug 28, 2023
@richtabor
Copy link
Member

Are you thinking of a way to omit from DFM (maybe an allowed list?), or adding a check in each command? The latter would be tough to mitigate as we continue adding commands.

@t-hamano
Copy link
Contributor Author

Are you thinking of a way to omit from DFM (maybe an allowed list?), or adding a check in each command? The latter would be tough to mitigate as we continue adding commands.

Not only in DFM mode, I think various conditions must be checked to see if the command is displayed. Therefore, I think we have to take the latter approach. The following recently submitted PRs are examples.

@ntsekouras
Copy link
Contributor

As far as I could find out, at least the following commands should not be displayed in DFM mode.

Should we hide these commands in DFM mode or just disable the mode? 🤔 Any thoughts @draganescu ?

@draganescu
Copy link
Contributor

draganescu commented Aug 29, 2023

So far we littered the codebase with code that disables DFM when the UI is no longer conforming to DFM mode.

As an UX I think so far it's the right path. As an implementation it's hard to discern if at the root is better (less flexible but prevents these UI bugs once and for all) or at the consumer (it's a few more lines to always toggle DFM off).

I think as far as UX goes we should leave the commands there and just turn off DFM for everything that causes UI bugs mostly because we want to improve DFM to a level where it won't have to be turned off anymore.

@t-hamano t-hamano changed the title [Tracking] Command Palette: Do not allow commands not expected in DFM mode [Tracking] Command Palette: Turn off DFM mode when a command not suitable for DFM mode is executed Aug 29, 2023
@t-hamano
Copy link
Contributor Author

t-hamano commented Aug 29, 2023

Thanks for the advice, @draganescu ! I have changed the title of the issue.

BTW, #52522 has also been reported that the Settings/Styles panel is unintentionally displayed in DFM mode.

@ntsekouras

This comment was marked as resolved.

@ntsekouras
Copy link
Contributor

Reset styles to defaults doesn't open a sidebar but the changes aren't saved and this might not be obvious.

Hide block breadcrumbs / Show block breadcrumbs: The breadcrumb is not always displayed in DFM mode. Is this command necessary?

I think breadcrumbs doesn't affect anything with DFM..

@t-hamano
Copy link
Contributor Author

t-hamano commented Sep 3, 2023

Thank you for addressing this issue, @ntsekouras!

Does this mean that this issue can be closed since DFM does not need to be considered for the following commands?

  • Hide block breadcrumbs / Show block breadcrumbs
  • Reset styles to defaults

@ntsekouras
Copy link
Contributor

I think we can close this now, yes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Commands /packages/commands [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

No branches or pull requests

4 participants