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

Refactor CalendarEdit component #23072

Merged
merged 2 commits into from
Aug 3, 2020
Merged

Refactor CalendarEdit component #23072

merged 2 commits into from
Aug 3, 2020

Conversation

pkvillanueva
Copy link
Contributor

@pkvillanueva pkvillanueva commented Jun 10, 2020

Description

Related to #22890

How has this been tested?

Tested this by inserting Calendar block in Post and Page post type. Compared the result with the previous component on the editor and frontend. Also, I compare the passed props on ServerSideRender and it looks fine.

Types of changes

Removed the getServerSideAttributes function, I don't think it is needed. It has memoization but it is not working, I think it is because we pass an object attributes param and the memize library does not make a shallow comparison. Edit: The memoization actually works with non-primitive data types, I was just testing it wrong.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

Copy link
Member

@ZebulanStanphill ZebulanStanphill left a comment

Choose a reason for hiding this comment

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

This looks good to merge. It just needs a rebase to get tests working.

@pkvillanueva
Copy link
Contributor Author

@ZebulanStanphill Done, thanks.

@ZebulanStanphill
Copy link
Member

Comically, it looks like the Static Analysis check is not working in master right now; it's failing on multiple PRs. I guess we'll have to wait a while and then rebase this again.

@ZebulanStanphill
Copy link
Member

I can't see how the failing test could possibly be related, so I'm going to assume it's just another intermittent failure that another rebase would fix, and I'm going to go ahead and merge this anyway.

Here's the failing test, for reference:

FAIL packages/e2e-tests/specs/editor/various/block-hierarchy-navigation.test.js (18.93s)
  ● Navigating the block hierarchy › should navigate using the block hierarchy dropdown menu

    expect(received).toMatchSnapshot()

    Snapshot name: `Navigating the block hierarchy should navigate using the block hierarchy dropdown menu 1`

    - Snapshot  - 3
    + Received  + 1

    @@ -8,10 +8,8 @@
      <!-- wp:column -->
      <div class="wp-block-column"></div>
      <!-- /wp:column -->

      <!-- wp:column -->
    - <div class="wp-block-column"><!-- wp:paragraph -->
    - <p>Third column</p>
    - <!-- /wp:paragraph --></div>
    + <div class="wp-block-column"></div>
      <!-- /wp:column --></div>
      <!-- /wp:columns -->

      71 | 		await page.keyboard.type( 'Third column' );
      72 | 
    > 73 | 		expect( await getEditedPostContent() ).toMatchSnapshot();
         | 		                                       ^
      74 | 	} );
      75 | 
      76 | 	it( 'should navigate block hierarchy using only the keyboard', async () => {

      at Object.<anonymous> (specs/editor/various/block-hierarchy-navigation.test.js:73:42)
          at runMicrotasks (<anonymous>)

@ZebulanStanphill ZebulanStanphill merged commit 8d6a3e6 into WordPress:master Aug 3, 2020
@github-actions github-actions bot added this to the Gutenberg 8.8 milestone Aug 3, 2020
@youknowriad
Copy link
Contributor

@ZebulanStanphill would be good to create an issue for the intermittent failure and ideally we should invesitage these as we notice them otherwise it just falls on one person when things get very bad 😬

@ZebulanStanphill
Copy link
Member

I was incorrect to say the test failure was "intermittent". It was happening every time, even after restarting the tests on the PR twice. But since it was not happening on other more recent PRs, and because I'm pretty sure it wasn't even the same failing test as the first time tests were failing, I assume the failure has already been fixed in master.

I'm currently watching the tests on master to see if they pass or not, and I'm about to rebase one of my PRs in order to confirm that this PR did not contain any regressions.

(I would have just rebased this branch myself, but since it's from a fork, I didn't think I could do that.)

@youknowriad
Copy link
Contributor

youknowriad commented Aug 3, 2020

Thanks for the clarification @ZebulanStanphill that sounds logic.

I think you can rebase almost all fork PRs unless the author unchecked a dedicated checkbox to forbid commits to his PR.

@ZebulanStanphill
Copy link
Member

ZebulanStanphill commented Aug 3, 2020

Well, that test failure didn't happen on master. Instead, a different test failed:

FAIL packages/e2e-tests/specs/experiments/template-part.test.js (47.978s)
  ● Template Part › Template part block › Should load customizations when in a template even if only the slug and theme attributes are set.

    TimeoutError: waiting for XPath "//button[contains(text(), "header")]" failed: timeout 30000ms exceeded

      46 | 				'.components-dropdown-menu__toggle[aria-label="Switch Template"]'
      47 | 			);
    > 48 | 			const switchToHeaderTemplatePartButton = await page.waitForXPath(
         | 			                                                    ^
      49 | 				'//button[contains(text(), "header")]'
      50 | 			);
      51 | 			await switchToHeaderTemplatePartButton.click();

      at new WaitTask (../../node_modules/puppeteer/lib/DOMWorld.js:486:34)
      at DOMWorld._waitForSelectorOrXPath (../../node_modules/puppeteer/lib/DOMWorld.js:420:26)
      at DOMWorld.waitForXPath (../../node_modules/puppeteer/lib/DOMWorld.js:393:21)
      at Frame.waitForXPath (../../node_modules/puppeteer/lib/FrameManager.js:575:51)
      at Frame.<anonymous> (../../node_modules/puppeteer/lib/helper.js:106:31)
      at Page.waitForXPath (../../node_modules/puppeteer/lib/Page.js:1015:33)
      at Object.<anonymous> (specs/experiments/template-part.test.js:48:56)
          at runMicrotasks (<anonymous>)

However, I noticed that this test was already failing on #24330 before I merged this PR, so that confirms that this latest test failure is not related to either PR.

@ZebulanStanphill
Copy link
Member

Whoops, linked to #24334 in my previous comment when I meant to link to #24330. I've edited it to fix that. 😛

@youknowriad
Copy link
Contributor

yes, template part is definitely an intermittent failure :(

@ZebulanStanphill
Copy link
Member

Yeah, I just rebased #21387 and the test isn't failing there, so it's definitely intermittent. 😞

I've opened #24336 to track this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Calendar Affects the Calendar Block [Package] Block library /packages/block-library [Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants