-
Notifications
You must be signed in to change notification settings - Fork 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
Gutenberg 7.3.0: Trigger E2E tests #38857
Changes from all commits
f83d8f9
eede374
1d7a231
c7f20f4
6b73537
0f3f84f
95a0aec
6d85607
d3f71ea
e8e6670
764e3b1
6d99dbb
383d066
2534eea
6e9a9ef
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -406,7 +406,7 @@ body.admin-bar:not( .is-fullscreen-mode ) .page-template-modal-screen-overlay { | |
> .editor-inner-blocks | ||
> .editor-block-list__layout | ||
> [data-type='core/column'] | ||
> .editor-block-list__block-edit | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as above (let's keep the old There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmmm I wonder if this is actually something we should be concerned at all with here. I can see several other occurrences of the non-legacy/newer formats in this and other files in the plugin. I feel a more concentrated effort ought to be carried out for this type of thing? There's at least 20+ occurrences of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not familiar with what's the backwards compatibility policy we have on the FSE plugin, but I think we tried to have it compatible with at least the 2 latest releases of WP Core (so FSE can work even if the Gutenberg plugin is not active). So I'd assume that any existing selector is currently working with WP 5.2 and we shouldn't worry about them. @Automattic/cylon or @Automattic/ajax might know more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok. My argument is just that if There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd defer to @noahtallen for a final call on this, but personally I don't think we'd need to be too worried about WP back compat. The plugin is meant to work with simple and atomic, third-party users are installing it on their own risk. See https://github.com/Automattic/wp-calypso/blob/master/apps/full-site-editing/full-site-editing-plugin/readme.txt#L32 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 for what @obenland said. We require the latest version of the Gutenberg dev plugin in a lot of cases, so we are fine updating everything to the most recent APIs as long as Atomic / Simple sites also have that. I think we removed the previous 2 WP version requirement a few months back. I agree with what @chriskmnds is saying about some of the syntax. We've been trying to keep up with newer CSS selectors as we've noticed things breaking, so we've also removed older classnames as well when we've made those updates. The only case I can think of is covering the gap between
Things could break in the period between when one of those is released and all of them is released. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the input folks. I believe we are aligned then. 🙂 p.s. it should be safe to apply these refactors to FSE at any point. It's just legacy selectors replaced with the new ones. I will prepare a separate PR. EDIT: Now handling these in #39035 |
||
> .block-editor-block-list__block-edit | ||
> div | ||
> .block-core-columns | ||
> .editor-inner-blocks { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -135,7 +135,7 @@ $color-accent-green: #31843f; | |
} | ||
|
||
// Same for the "add item" input. | ||
.editor-block-list__block .wp-block-a8c-todo .add-new-item { | ||
.block-editor-block-list__block .wp-block-a8c-todo .add-new-item { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mmtr anyone to nudge about this? 😀 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't know who owns o2 blocks, but they are used in P2s so we can assume that latest Gutenberg will be available. This will also require to generate a new build and upload it to WP.com but I think @ockham is familiar with the process. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as above (let's keep the old |
||
width: 100%; | ||
margin-right: 6px; | ||
padding: 7.5px 8px; | ||
|
@@ -154,8 +154,8 @@ body:not( .wp-admin ) .gutenberg { | |
display: none; | ||
} | ||
|
||
.editor-block-list__block.is-selected, | ||
.editor-block-list__block.is-typing { | ||
.block-editor-block-list__block.is-selected, | ||
.block-editor-block-list__block.is-typing { | ||
.wp-block-a8c-todo .add-new-todo-item-form { | ||
display: flex; | ||
} | ||
|
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 will require a new FSE release @Automattic/cylon
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.
I think we are aiming to update Monday, will that work with this change?
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.
If the CSS class is changing, it might be a good idea to keep the old one to handle the case where a user is still on an older Gutenberg version (unless this Gutenberg update has already been pushed to everyone on simple and atomic)
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.
I believe these refactors happened a while back in Gutenberg. This seems like the relevant PR: WordPress/gutenberg#14420
Legacy support was also dropped in WordPress/gutenberg#19050
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.
Ah yeah, removing legacy support would certainly do it. Thanks for adding the fix! By the way, how did you find that these broke?
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.
Not sure I recall exactly, but probably something I spotted while looking into some of the e2e test failures. This in particular may have led me to it (meaning, to search for other references): https://github.com/Automattic/wp-calypso/pull/38857/files#diff-926c3450baa62272252cb16253634da9L27-R27
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.
We might still need to keep the old
.editor-
selectors in order to support old versions of Gutenberg. In any case, since these changes are unrelated to the E2E fixes, I'd move them to a separate 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.
Ok, I can create a new PR with these changes.
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.
See my comment here please before I proceed: https://github.com/Automattic/wp-calypso/pull/38857/files#r369510379 I may be missing something.