-
Notifications
You must be signed in to change notification settings - Fork 11
Reconfigure Video
options to optimize for user's settings around autoplay
#276
Conversation
* main: (93 commits) Enforces where React component static properties should be positioned (#429) Node 12 (#428) 3.3.1 Prepare 3.3.1 The `edit.css` prop is optional when displaying `Edit` in `CodeSnippet` (#421) 3.3.0 Prepare 3.3.0 Bump elliptic from 6.5.3 to 6.5.4 (#417) When `maxHeight` is set, move `Edit` buttons in `CodeSnippet` above the code (#420) Update CONTRIBUTING.md (#418) 3.2.0 update version number in changelog Add pricing theme to Note component (#416) 3.1.0 Prepare 3.1.0 Make `filename` required for `CodeSnippetTitle` (#413) Update package-lock.json (#412) Make `css` optional in Edit and CodeSnippet components (#293) Remove HelpPage, NavigationDropdown, SectionedNavigation, TopbarSticker, Topbar components (#407) Update prism (#411) ...
* 4.0.0: Create `prepareSitemap` Batfish helper to improve sitemap (#427) Add eslint-plugin-jsx-a11y (#435) 🔧 Run prettier Improve color contrast on code and anchor elements (#434) Replace stateless components with pure components (#414) Update dependencies (#433) Make code toggles in test cases interactive or label if they are not (#432) Prepare 4.0.0 changelog
* 4.0.0: Replace `id` with `aria-label` in VimeoPlayImage (#438)
* 4.0.0: Make `filename` optional for `CodeSnippetTitle`
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like a really smartly designed video component, @katydecorah! I was a little confused by autoplay and left a comment inline. When I tested locally, the basic example was autoplaying by default. Looking at video.js
, that seems like the expected behavior, but the code comments and changelog entry suggest it shouldn't be. Am I misunderstanding that test case?
src/components/video/video.js
Outdated
</p> | ||
</video> | ||
</div> | ||
); | ||
} | ||
} | ||
|
||
// default props are only set if the user does not prefer reduced motion |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great feature!
</p> | ||
</video> | ||
</div> | ||
); | ||
} | ||
} | ||
|
||
// default props are only set if the user does not prefer reduced motion | ||
Video.defaultProps = { | ||
autoplay: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should autoplay be false
by default?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@danswick I can help improve the wording, but here's the gist:
- The
state
by default disables autoplay. We assume that the user will prefer reduced motion. - Then in componentDidMount, we check the users's preferences:
- If the user prefers reduced motion, the component will not use default props and will continue to disable auto play.
- If the user does not prefer reduced motion, then the component will use default props to determine if the video should be autoplayed.
This gives the developer flexibility is turning autoplay on or off. But ultimately, we will default to the user's preferences. Where default props = developer preferences, state = user preferences.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for tweaking the comment language, @katydecorah! Looks great to me 🥞
* 4.0.0: Reconfigure `Video` options to optimize for user's settings around autoplay (#276)
* Prepare 4.0.0 changelog * Make code toggles in test cases interactive or label if they are not (#432) * Update dependencies (#433) * Update dependencies * Update package-lock.json * Update rehype-slug * Fix links * Update page-layout.md * Update snapshots * Replace stateless components with pure components (#414) * Use PureComponent on stateless components * Update package-lock.json * 4.0.0-alpha.0 * Use PureComponents where components do not require rerenders * 4.0.0-alpha.1 * prefer-const * Update CHANGELOG.md * Make more components pure * 4.0.0-alpha.2 * 4.0.0-alpha.3 * Improve color contrast on code and anchor elements (#434) * Expand a code color contrast improvement * Increase color contrast on .color-gray code * Increase color contrast for prism's .token.regex,.token.important * Clean up test cases * Update CHANGELOG.md * Clean up test cases * Clean up test cases * Update docs/src/pages/guides/a11y.md Co-authored-by: Colleen McGinnis <colleen.mcginnis@mapbox.com> * Reduce changes Co-authored-by: Colleen McGinnis <colleen.mcginnis@mapbox.com> * 🔧 Run prettier * Add eslint-plugin-jsx-a11y (#435) * Add eslint-plugin-jsx-a11y * Fix props * Update CHANGELOG.md * Update search.test.js.snap * Update eslint * Update eslint * Create `prepareSitemap` Batfish helper to improve sitemap (#427) * Create prepareSitemap Batfish helper * Update CHANGELOG.md * Clean up, add `docsPath`, and `outputDirectory` * Exclude `splitPage` from sitemap * Assert sitemap * Extract urls * Sort sitemap * Remove snapshot * Update sitemap.test.js * Update batfish-helpers.md * Update batfish-helpers.md * Copyedit * Update package-lock.json * Replace `id` with `aria-label` in VimeoPlayImage (#438) * Replace `id` with aria-label to prevent duplicated id * Update CHANGELOG.md * Make `filename` optional for `CodeSnippetTitle` * Make "With token" `DemoIframe` a no render test case (#440) * Make "With token" `DemoIframe` a no render test case * Rename * Use passive event listeners on `Search`, `NumberedCodeSnippet`, and `OnThisPage` to improve performance (#437) * Use passive event listeners on `Search`, `NumberedCodeSnippet`, and `OnThisPage` to improve performance. * Update CHANGELOG.md * Reconfigure `Video` options to optimize for user's settings around autoplay (#276) * show video controls on hover; show play button for browsers with disabled autoplay * Merge branch 'main' into video-controls * main: Add the catalog site (#137) Delete dependabot.yml Only direct dependencies Create dependabot.yml (#296) Update Travis bad Update devDependencies (#294) * Add example * More comments * Reduce changes * eslint and fix button styles * Clean up button * Set playsInline to true if autoPlay is true; make `muted` a prop * Update test cases description to indicate the autoplay state * Update description * Update snapshots * Update CHANGELOG.md * Create `playsInline` prop * Append hash to src to enable preview on iOS * Update video.js * Adds optional `poster` prop * Remove hack * Add poster example * Fix paths * Update descriptions * Apply suggestions from code review * Load Search with a facade (#430) * Load Search with a facade * Add @loadable/babel-plugin * Update CHANGELOG.md * Fix logic * Update search.test.js.snap * Define development babel env to prevent `@loadable/babel-plugin` from loading on start (because it fails) * Adjusting snapshots * Revert "Adjusting snapshots" This reverts commit d2e7fd9. * Clean up props * Comments * Reduce changes * Reduce changes * Reduce changes * Reduce changes * Clean up props * Use SearchInput in SearchBox * Use SearchButton in SearchBox * Reduce changes * Add missing props * Consolidate wrapper * 4.0.0-alpha.4 * Try dynamic import * Update package-lock.json * 4.0.0-alpha.5 * Update package-lock.json * Reduce changes * Reorganize files * 4.0.0-alpha.6 * Switch to noRenderCase * Reduce changes * Add passive event listener * Allow autofocus * Enable react/jsx-no-bind (#442) * jsx-no-bind * Update CHANGELOG.md * Simplify, reduce changes * The `NumberedCodeSnippet` prop `onCopy` is deprecated and will automatically send a track event to Segment * Remove unused onCopy * Simplify * Add onCopy * Create onCopy function * Update src/components/code-snippet/on-copy.js * Update dependencies * Update caniuse * Update devDependencies * Update CHANGELOG.md * Fix NavigationAccordion tests (#443) * 4.0.0-alpha.7 * Fix prepareSitemap: append index.html to files ending in index.js (#444) * 4.0.0-alpha.8 * Update batfish * Update caniuse-lite * Fix props on search toggle (#448) * Fix props on search toggle * 4.0.0-alpha.9 * For searches with no results, retry if Swiftype thinks there is a spelling error (#451) * pass through swiftype API spelling option * move prettier'd comment * Update search-ui, downshift * Update caniuse-lite * Update @sentry/browser * Send search results with no queries to Sentry (#452) * Send search results with no queries to Sentry * Update CHANGELOG.md * Update prettier * Fix Sentry test * 4.0.0-alpha.10 * Switch Sentry environment back to `main` * Fix Search/Sentry, only send when results change * 4.0.0 * Clean up Co-authored-by: Colleen McGinnis <colleen.mcginnis@mapbox.com> Co-authored-by: Dan Swick <dan.swick@gmail.com>
This branch merges into #431
This PR makes some changes to how we handle autoplay, loop, and the displaying of controls in the
Video
component. Previously we handled users who prefer reduce motion by disabling loop, autoplay, and showing the controls. We did not take into account browser settings that would disable autoplay, so this PR will.The goal is to give preference to the user's settings first:
Fixes #275
Screenshots
How to test
Interact with /Video or /Phone test cases:
QA checklist
7.0.0
.Open the test cases app locally on: