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

Gutenberg 7.3.0: Trigger E2E tests #38857

Merged
merged 15 commits into from
Jan 24, 2020
Merged

Gutenberg 7.3.0: Trigger E2E tests #38857

merged 15 commits into from
Jan 24, 2020

Conversation

chriskmnds
Copy link
Contributor

@chriskmnds chriskmnds commented Jan 15, 2020

E2E integration tests against Gutenberg Edge.

@chriskmnds chriskmnds added DO NOT MERGE [Status] Needs e2e Testing Gutenberg Edge Run e2e tests with a special site that is running the latest Gutenberg plugin snapshot labels Jan 15, 2020
@chriskmnds chriskmnds self-assigned this Jan 15, 2020
@matticbot
Copy link
Contributor

@matticbot
Copy link
Contributor

This PR does not affect the size of JS and CSS bundles shipped to the user's browser.

Generated by performance advisor bot at iscalypsofastyet.com.

@chriskmnds chriskmnds requested review from a team as code owners January 16, 2020 10:44
@@ -65,10 +65,10 @@
border-color: rgba( #425863, 0.4 ); // Gutenberg $dark-opacity-light-800
}

.editor-block-list__block:first-child & {
.block-editor-block-list__block:first-child & {
Copy link
Member

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

Copy link
Contributor

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?

Copy link
Contributor

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)

Copy link
Contributor Author

@chriskmnds chriskmnds Jan 17, 2020

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

Copy link
Contributor

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?

Copy link
Contributor Author

@chriskmnds chriskmnds Jan 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how did you find that these broke

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

Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

@chriskmnds chriskmnds added [Status] Needs e2e Testing Gutenberg Edge Run e2e tests with a special site that is running the latest Gutenberg plugin snapshot and removed [Status] Needs e2e Testing Gutenberg Edge Run e2e tests with a special site that is running the latest Gutenberg plugin snapshot labels Jan 16, 2020
@@ -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 {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mmtr anyone to nudge about this? 😀

Copy link
Member

@mmtr mmtr Jan 20, 2020

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above (let's keep the old .editor- selectors and move these changes to a separate PR).

@@ -225,7 +235,7 @@ export default class GutenbergEditorComponent extends AsyncBaseContainer {
.toLowerCase() }`
);
const insertedBlockSelector = By.css(
`.block-editor-block-list__block.is-selected[aria-label*='Block: ${ name }']`
`.block-editor-block-list__block.${ selectedBlockConfirmClass }[aria-label*='${ name } Block']`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's going on here is that we're inserting a block that contains child blocks. When normally the entire block would then be selected, it's only the first child block in this case:

pricing table

Ideally, we'll find a way to make our selector accomodate for both cases 🤔

If we can't, I'd still suggest maybe handling the 'child block selected' case not through a generic-sounding selectedBlockConfirmClass, but maybe through a bool that determines whether we're dealing with a 'normal' block, or with one that has inner blocks, and set the relevant selector class to either is-selected or has-child-selected, depending on the value of that bool (so the code represents what's going on maybe a bit more intuitively).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, sounds like a good idea. I'll do a cleanup in the end to nail things down. :-)

@ockham ockham added [Status] Needs e2e Testing Gutenberg Edge Run e2e tests with a special site that is running the latest Gutenberg plugin snapshot and removed [Status] Needs e2e Testing Gutenberg Edge Run e2e tests with a special site that is running the latest Gutenberg plugin snapshot labels Jan 21, 2020
@@ -65,10 +65,10 @@
border-color: rgba( #425863, 0.4 ); // Gutenberg $dark-opacity-light-800
}

.editor-block-list__block:first-child & {
.block-editor-block-list__block:first-child & {
Copy link
Member

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.

@@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above (let's keep the old .editor- selectors and move these changes to a separate PR).

Copy link
Contributor Author

@chriskmnds chriskmnds Jan 22, 2020

Choose a reason for hiding this comment

The 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 block-editor-block prefixed selectors in the plugin that could potentially be candidates for legacy-support. 🤔

Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

@chriskmnds chriskmnds Jan 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. My argument is just that if block-editor-block__x won't work here due to missing support for older browsers, then it won't work anywhere. I am not sure if it makes a lot of sense to add support here and leave it hanging loose elsewhere. I feel a more holistic effort is appropriate for this - assuming that's the case! (last I checked it was 20+ candidate selectors). Then again I may be wrong (on approach at least). Let's see what @Automattic/cylon @Automattic/ajax are thinking. 🙂

Copy link
Member

Choose a reason for hiding this comment

The 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

Copy link
Contributor

Choose a reason for hiding this comment

The 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

  • FSE plugin update is released with new classnames
  • Gutenberg plugin update is released to Simple sites
  • Gutenberg plugin update is released to Atomic sites.

Things could break in the period between when one of those is released and all of them is released.

Copy link
Contributor Author

@chriskmnds chriskmnds Jan 23, 2020

Choose a reason for hiding this comment

The 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

@@ -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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above (let's keep the old .editor- selectors and move these changes to a separate PR).

@ockham ockham added [Status] Needs e2e Testing Gutenberg Edge Run e2e tests with a special site that is running the latest Gutenberg plugin snapshot and removed [Status] Needs e2e Testing Gutenberg Edge Run e2e tests with a special site that is running the latest Gutenberg plugin snapshot labels Jan 23, 2020
Copy link
Contributor

@ockham ockham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DO NOT MERGE [Status] Needs e2e Testing Gutenberg Edge Run e2e tests with a special site that is running the latest Gutenberg plugin snapshot
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants