Skip to content
This repository has been archived by the owner on Jun 12, 2024. It is now read-only.

Fix Video build #186

Merged
merged 5 commits into from
Sep 16, 2019
Merged

Fix Video build #186

merged 5 commits into from
Sep 16, 2019

Conversation

colleenmcginnis
Copy link
Contributor

When rolling out v0.21.1 to all repos, I came across a build error related to the Video component (catching on this use of window). I did two things in this PR to address it:

  • Use /mr-ui's getWindow (for consistency).
  • Check for the window within componentDidMount (this seems to be the important part).
    • I chose to store the videoProps (which depend on the results the part that uses getWindow) in state because that seemed to be a pattern used in mr-ui components that use getWindow. Up for other suggestions, though!

I tested this by building and installing this locally in /ios-sdk and /android-docs. I was able to build both sites successfully and the resulting static site worked as expected.

@katydecorah
Copy link
Contributor

🤦‍♂ I didn't double check my work in the last PR and the code to evaluate the window is wrong. We can keep the original code, but use:

const reducedMotion =
      typeof window !== 'undefined'
        ? window.matchMedia('(prefers-reduced-motion: reduce)').matches
        : false;

I know I had implemented the use of @mapbox/mr-ui/utils/get-window in the feedback module, but I'm rethinking that since if the window is not defined it will throw an error. This isn't necessarily something we want as if the window is undefined, we'd rather it fail silently. I'll create a PR to address this in the feedback module.

Copy link
Contributor

@katydecorah katydecorah left a comment

Choose a reason for hiding this comment

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

🌹 thanks colleen! sorry for the confusion 🙇‍♀

@colleenmcginnis colleenmcginnis merged commit 9fd8b3f into master Sep 16, 2019
@colleenmcginnis colleenmcginnis deleted the fix-video-build branch September 16, 2019 18:43
katydecorah pushed a commit that referenced this pull request Sep 16, 2019
katydecorah pushed a commit that referenced this pull request Sep 16, 2019
* master:
  Fix Video build (#186)
katydecorah pushed a commit that referenced this pull request Nov 6, 2019
* master:
  create new theme for search button (#189)
  Add glossary components (#192)
  prepare 0.21.3
  Adds a CSS highlighter option to highlight component (#191)
  prepare 0.21.2
  rename tests directory (#185)
  [Feedback] remove get-window (#187)
  Fix Video build (#186)
  Prepare 0.21.1
  [Feedback] send `environment` and `location` with feedback (#184)
  prepare 0.21.0
  Bump eslint-utils from 1.4.0 to 1.4.2 (#174)
  [Feedback] collect browser, operating system, preferredLanguage (#183)
  iOS and Android toggleable code snippets (#179)
  move highlight helper function to highlight component (#180)
  add missing video/index.js file (#181)
katydecorah pushed a commit that referenced this pull request Jan 7, 2020
* master:
  prepare 0.21.2
  rename tests directory (#185)
  [Feedback] remove get-window (#187)
  Fix Video build (#186)
  Prepare 0.21.1
  [Feedback] send `environment` and `location` with feedback (#184)
  prepare 0.21.0
  Bump eslint-utils from 1.4.0 to 1.4.2 (#174)
  [Feedback] collect browser, operating system, preferredLanguage (#183)
  iOS and Android toggleable code snippets (#179)
  move highlight helper function to highlight component (#180)
  add missing video/index.js file (#181)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants