-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Editor: Migrate and unify the panel preferences #57529
Conversation
Size Change: +497 B (0%) Total Size: 1.69 MB
ℹ️ View Unchanged
|
To test I rebased to fix conflicts in migrations/preferences-package-data/convert-editor-settings.js - I hope you don't mind! LGTM All settings work except the accessibility labels in the site editor, but this is a trunk issue. I cleared local storage 🤔 I'm going to have a quick look. 2024-01-05.09.01.52.mp4 |
PR: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM1
Footnotes
-
If it looks good to @andrewserong 😄 ↩
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking pretty good to me, too! It seems that the test failure for should show the New Post page in Gutenberg
is happening locally, too. From manually testing it looks like with this PR applied, the Summary panel is defaulting to closed for that test, whereas on trunk it's open, so the check to see if the Post Format combobox fails here. Any idea why this PR would alter the open state for that panel?
Here are a couple of screenshots from manual testing locally:
Trunk (summary panel is open) | This PR (summary panel is closed |
---|---|
As for the behaviour in this PR, it's all working well for me manually:
✅ Settings set on trunk
are persisted / migrated correctly when applying this PR
✅ Settings set in the post editor are reflected in the site editor and vice versa
One detail that isn't a blocker for this PR, but that I found a little confusing at first, is that which settings are visible is dependent on context. So if you open the template editor, the Document settings area is empty, but when you edit a page within the site editor, you can then see the settings available. This could make it challenging for folks wanting to quickly update settings if they're sometimes visible and sometimes not:
Preferences while editing a template | Preferences while editing a page |
---|---|
Would it be worth exposing all the settings available in the site editor so that folks don't need to be on a particular part of the site editor in order to see all the available settings?
I think showing all the possible panels all the time in the preferences modal has a downside of potentially confusing users: They'll try to enable a panel but that panel doesn't show up because the CPT doesn't support it for a reason. So I'm not sure it's the best path there. I'm going to check why the default values are different, it seems a bit weird :) |
Ok I've found that I was missing some initializations. Let's see if it fixes everything. |
b5b5a69
to
0178764
Compare
Related #52632
Similar to #57468
What?
This PR continues the work on the great unification between post and site editors. In this PR we're unifying the "Document settings panels" preference. If the user disables a panel in the post editor, the panel should still be hidden in the site editor.
This PR also adds the panels to the "preferences modal" in the site editor to match the post editor preferences modal.
Testing instructions