Skip to content

Block: try merging effects#19617

Merged
ellatrix merged 2 commits intomasterfrom
try/merge-block-effects
Jan 14, 2020
Merged

Block: try merging effects#19617
ellatrix merged 2 commits intomasterfrom
try/merge-block-effects

Conversation

@ellatrix
Copy link
Copy Markdown
Member

@ellatrix ellatrix commented Jan 14, 2020 •
edited
Loading

Description

Currently there's a useEffect and useLayoutEffect to set focus on the block. I'm curious to see if any tests will fail if we take away useLayoutEffect.

I also merged the effect setting focus for multi selection.

How has this been tested?

Screenshots

Types of changes

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

youknowriad
youknowriad approved these changes Jan 14, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

It seems to work as intended.

@ellatrix
Copy link
Copy Markdown
Member Author

ellatrix commented Jan 14, 2020

Thanks! I also merged the multi-selection focus effect.

@ellatrix
Copy link
Copy Markdown
Member Author

ellatrix commented Jan 14, 2020

Looks like this will make typing another 4% faster.


Your branch is up to date with 'origin/master'.
Ellas-MacBook-Pro:gutenberg ella$ npm run test-performance

> gutenberg@7.2.0 test-performance /Users/ella/gutenberg
> wp-scripts test-e2e --config packages/e2e-tests/jest.performance.config.js

 PASS  packages/e2e-tests/specs/performance/performance.test.js (20.601s)
  Performance
    ✓ 1000 paragraphs (17754ms)

Test Suites: 1 passed, 1 total
Tests:       1 passed, 1 total
Snapshots:   0 total
Time:        20.704s
Ran all test suites.

Average time to load: 4506ms
Average time to DOM content load: 4468ms
Average time to type character: 31.105ms
Slowest time to type character: 73ms
Fastest time to type character: 17ms
		
Ellas-MacBook-Pro:gutenberg ella$ git checkout -
M	packages/e2e-tests/config/setup-test-framework.js
Switched to branch 'try/merge-block-effects'
Ellas-MacBook-Pro:gutenberg ella$ npm run test-performance

> gutenberg@7.2.0 test-performance /Users/ella/gutenberg
> wp-scripts test-e2e --config packages/e2e-tests/jest.performance.config.js

 PASS  packages/e2e-tests/specs/performance/performance.test.js (20.276s)
  Performance
    ✓ 1000 paragraphs (17473ms)

Test Suites: 1 passed, 1 total
Tests:       1 passed, 1 total
Snapshots:   0 total
Time:        20.381s, estimated 21s
Ran all test suites.

Average time to load: 4568ms
Average time to DOM content load: 4270ms
Average time to type character: 29.76ms
Slowest time to type character: 112ms
Fastest time to type character: 17ms

@ellatrix ellatrix merged commit 5338c47 into master Jan 14, 2020
@ellatrix ellatrix deleted the try/merge-block-effects branch January 14, 2020 10:09
@ellatrix ellatrix added this to the Gutenberg 7.3 milestone Jan 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@youknowriad youknowriad youknowriad approved these changes

@talldan talldan Awaiting requested review from talldan

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

Gutenberg 7.3

Development

Successfully merging this pull request may close these issues.

2 participants

@ellatrix @youknowriad