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

♻️ Bento: Internal components should not need ...rest #35693

Merged
merged 3 commits into from
Aug 18, 2021

Conversation

caroqliu
Copy link
Contributor

... because we know exactly what props are being passed to it, and should propagate them intentionally when relevant rather than blindly.

That's how we get the outsetArrows issue described in #35553:

* Warning: React does not recognize the `outsetArrows` prop on a DOM element. If you intentionally want it to appear in the DOM as a custom attribute, spell it as lowercase `outsetarrows` instead. If you accidentally passed it from a parent component, remove it from the DOM element.

The internal components targeted include:

  • BaseCarousel's helpers Scroller, Arrow, and DefaultArrow
  • StreamGallery's helper DefaultArrow
  • LightboxGalleryProviders's helpers CloseButtonIcon, NavButtonIcon, and ToggleViewIcon
  • Lightbox's helper ScreenReaderCloseButton
  • SocialShare's helper SocialShareIcon

/to @developit

@caroqliu caroqliu requested a review from alanorozco August 16, 2021 21:02
@amp-owners-bot
Copy link

Hey @erwinmombay, @jridgewell! These files were changed:

build-system/babel-plugins/babel-plugin-transform-simple-array-destructure/index.js
build-system/babel-plugins/babel-plugin-transform-simple-array-destructure/test/fixtures/transform/deep-array/input.js
build-system/babel-plugins/babel-plugin-transform-simple-array-destructure/test/fixtures/transform/deep-array/options.json
build-system/babel-plugins/babel-plugin-transform-simple-array-destructure/test/fixtures/transform/deep/input.js
build-system/babel-plugins/babel-plugin-transform-simple-array-destructure/test/fixtures/transform/deep/options.json
build-system/babel-plugins/babel-plugin-transform-simple-array-destructure/test/fixtures/transform/param/input.js
build-system/babel-plugins/babel-plugin-transform-simple-array-destructure/test/fixtures/transform/param/options.json
build-system/babel-plugins/babel-plugin-transform-simple-array-destructure/test/fixtures/transform/skip-element/input.js
build-system/babel-plugins/babel-plugin-transform-simple-array-destructure/test/fixtures/transform/skip-element/options.json
build-system/babel-plugins/babel-plugin-transform-simple-array-destructure/test/fixtures/transform/skip-element/output.js
build-system/babel-plugins/babel-plugin-transform-simple-array-destructure/test/fixtures/transform/variable-declarations/input.js
build-system/babel-plugins/babel-plugin-transform-simple-array-destructure/test/fixtures/transform/variable-declarations/options.json
+3 more

Copy link

@developit developit left a comment

Choose a reason for hiding this comment

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

Looks good! Does Bento have any sort of memory tracking/benchmarking set up? I would expect this to show a decent reduction in memory footprint due to the reduced cloning.

Copy link
Member

@alanorozco alanorozco left a comment

Choose a reason for hiding this comment

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

Thanks!

@caroqliu
Copy link
Contributor Author

Does Bento have any sort of memory tracking/benchmarking set up?

Good idea, no memory tracking/benchmarking set up currently, but I'll make a note to look into this. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants