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

Block: try merging effects #19617

Merged
merged 2 commits into from
Jan 14, 2020
Merged

Block: try merging effects #19617

merged 2 commits into from
Jan 14, 2020

Conversation

ellatrix
Copy link
Member

@ellatrix ellatrix commented Jan 14, 2020

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

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

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

@ellatrix
Copy link
Member Author

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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants