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: remove animation component so it is just a hook #22936

Merged
merged 3 commits into from
Jun 9, 2020

Conversation

ellatrix
Copy link
Member

@ellatrix ellatrix commented Jun 5, 2020

Description

I'm pretty excited about this PR. :) This PR removes the Animated component from the block tree by applying styles to the ref on frame instead. This way, the whole animation is now reduced to a simple hook.

For background, see pmndrs/react-spring#1042.

Why is this exciting? This could potentially allow us to write a simple useBlock( ref ) component to be used within blocks. Such a hook would be very flexible to use compared to a component or HoC.

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.

@ellatrix ellatrix requested a review from youknowriad as a code owner June 5, 2020 13:28
@github-actions
Copy link

github-actions bot commented Jun 5, 2020

Size Change: +1.7 kB (0%)

Total Size: 1.12 MB

Filename Size Change
build/autop/index.js 2.83 kB +1 B
build/block-directory/index.js 6.75 kB +1 B
build/block-editor/index.js 106 kB +12 B (0%)
build/block-library/editor-rtl.css 7.87 kB -4 B (0%)
build/block-library/editor.css 7.87 kB -3 B (0%)
build/block-library/index.js 127 kB +150 B (0%)
build/blocks/index.js 48.1 kB -37 B (0%)
build/components/index.js 194 kB +251 B (0%)
build/compose/index.js 9.31 kB +1 B
build/core-data/index.js 11.4 kB -2 B (0%)
build/data/index.js 8.45 kB -2 B (0%)
build/date/index.js 5.47 kB +1 B
build/edit-navigation/index.js 8.25 kB +1 B
build/edit-post/index.js 303 kB +295 B (0%)
build/edit-site/index.js 15.5 kB +532 B (3%)
build/edit-widgets/index.js 9.33 kB +505 B (5%) 🔍
build/editor/index.js 44.7 kB +6 B (0%)
build/format-library/index.js 7.72 kB +1 B
build/list-reusable-blocks/index.js 3.12 kB -1 B
build/nux/index.js 3.41 kB +1 B
build/plugins/index.js 2.56 kB -1 B
build/rich-text/index.js 14.8 kB -2 B (0%)
build/server-side-render/index.js 2.68 kB -2 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.14 kB 0 B
build/annotations/index.js 3.62 kB 0 B
build/api-fetch/index.js 3.4 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/style-rtl.css 892 B 0 B
build/block-directory/style.css 892 B 0 B
build/block-editor/style-rtl.css 11.4 kB 0 B
build/block-editor/style.css 11.4 kB 0 B
build/block-library/style-rtl.css 7.72 kB 0 B
build/block-library/style.css 7.72 kB 0 B
build/block-library/theme-rtl.css 684 B 0 B
build/block-library/theme.css 686 B 0 B
build/block-serialization-default-parser/index.js 1.88 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/components/style-rtl.css 19.5 kB 0 B
build/components/style.css 19.5 kB 0 B
build/data-controls/index.js 1.29 kB 0 B
build/deprecated/index.js 771 B 0 B
build/dom-ready/index.js 568 B 0 B
build/dom/index.js 3.17 kB 0 B
build/edit-navigation/style-rtl.css 918 B 0 B
build/edit-navigation/style.css 919 B 0 B
build/edit-post/style-rtl.css 5.43 kB 0 B
build/edit-post/style.css 5.43 kB 0 B
build/edit-site/style-rtl.css 2.96 kB 0 B
build/edit-site/style.css 2.96 kB 0 B
build/edit-widgets/style-rtl.css 2.4 kB 0 B
build/edit-widgets/style.css 2.4 kB 0 B
build/editor/editor-styles-rtl.css 425 B 0 B
build/editor/editor-styles.css 428 B 0 B
build/editor/style-rtl.css 4.26 kB 0 B
build/editor/style.css 4.27 kB 0 B
build/element/index.js 4.65 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/style-rtl.css 502 B 0 B
build/format-library/style.css 502 B 0 B
build/hooks/index.js 2.13 kB 0 B
build/html-entities/index.js 621 B 0 B
build/i18n/index.js 3.56 kB 0 B
build/is-shallow-equal/index.js 711 B 0 B
build/keyboard-shortcuts/index.js 2.52 kB 0 B
build/keycodes/index.js 1.94 kB 0 B
build/list-reusable-blocks/style-rtl.css 226 B 0 B
build/list-reusable-blocks/style.css 226 B 0 B
build/media-utils/index.js 5.3 kB 0 B
build/notices/index.js 1.79 kB 0 B
build/nux/style-rtl.css 616 B 0 B
build/nux/style.css 613 B 0 B
build/primitives/index.js 1.5 kB 0 B
build/priority-queue/index.js 789 B 0 B
build/redux-routine/index.js 2.85 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.28 kB 0 B
build/url/index.js 4.06 kB 0 B
build/viewport/index.js 1.85 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.17 kB 0 B

compressed-size-action

ref.current.style.transform =
x === 0 && y === 0 ? null : `translate3d(${ x }px,${ y }px,0)`;
ref.current.style.zIndex =
! isSelected || ( x === 0 && y === 0 ) ? null : '1';
Copy link
Contributor

Choose a reason for hiding this comment

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

hah :) so you're recreating react-spring. It would be interesting to see how this differs from React spring internally. I mean shouldn't we be able to pass a ref for react-spring to do that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, just read the use-spring issue. i wonder what's different between this and applyAnimatedValues calls? Also should we upgrade?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not really as far as recreating. It still provides the values, we just take render it here instead of passing instructions on how to render it for us. Ideally it would be great if we could just pass the ref and instructions to useSpring, but there's no big difference.

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, so it looks like this is the function that runs in the library https://github.com/react-spring/react-spring/blob/master/src/targets/web/globals.ts#L82-L127 which is pretty similar to what you did for the "styles" here.

Copy link
Contributor

Choose a reason for hiding this comment

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

While testing though, it seems the animation is broken for me (blocks overlap)

Copy link
Member

Choose a reason for hiding this comment

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

I tested on Arch Linux using a browser based on Chromium 80, and the block movement animations all seem fine to me. I also tested in Firefox Developer Edition 78.0b2 and everything worked there as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's weird, they overlap for me regardless of the browser (Chrome and Safari)

Copy link
Member Author

Choose a reason for hiding this comment

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

@youknowriad Did you do a full refresh etc? It works fine for me. I also tried switching branches a few times, but I can't reproduce any overlap issues

Copy link
Contributor

Choose a reason for hiding this comment

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

So I just tested for the third time and this time it worked. Sorry for delaying that.

Copy link
Member Author

Choose a reason for hiding this comment

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

No worries :) I'm glad it works now

@ZebulanStanphill ZebulanStanphill added [Feature] Blocks Overall functionality of blocks [Package] Block editor /packages/block-editor [Type] Enhancement A suggestion for improvement. labels Jun 5, 2020
@ellatrix
Copy link
Member Author

ellatrix commented Jun 9, 2020

Thanks!

@ellatrix ellatrix merged commit c9bffda into master Jun 9, 2020
@ellatrix ellatrix deleted the try/remove-animation-component branch June 9, 2020 12:11
@github-actions github-actions bot added this to the Gutenberg 8.4 milestone Jun 9, 2020
@talldan talldan assigned ellatrix and unassigned talldan Jun 11, 2020
This was referenced Jun 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Blocks Overall functionality of blocks [Package] Block editor /packages/block-editor [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants