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

Create GL Support wrapper #166

Merged
merged 12 commits into from
Aug 14, 2019
Merged

Create GL Support wrapper #166

merged 12 commits into from
Aug 14, 2019

Conversation

katydecorah
Copy link
Contributor

This component is a wrapper where we can pass HTML/Component to it as a prop. The wrapper will decide if the browser supports webgl. If it does then it'll show whatever code you passed to it, if it doesn't then it'll display a note with a warning.

Refs https://github.com/mapbox/documentation/issues/236

WebGL supported:
image

WebGL unsupported:
image

How to test:

  1. npm install
  2. npm start
  3. Open the component in a browser you know to have GL support (your regular browser) you should see a typing cat.
  4. Open the component in a browser that doesn't support GL or disable GL support. Here's how you can disable GL in Firefox:
    • Open the browser settings page. To do this, type in the address bar: about:config.
    • In the search box, type in: webgl.
    • Find line “webgl.disabled”. It has the “false” value. It is necessary to change it to “true”. To do this simply click twice on this line.

OR!

  1. Visit /help/how-mapbox-works/ on staging
  2. Test that page in a browser with GL and without GL support.

🐌 No rush on review 🐌

@katydecorah katydecorah requested a review from a team August 8, 2019 15:59
Copy link
Contributor

@colleenmcginnis colleenmcginnis left a comment

Choose a reason for hiding this comment

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

Hmmm I don't love this pattern, but I don't have a good reason why:

<GLWrapper map={<DemoIframe src="/help/demos/how-mapbox-works/map-design.html" />} />

If we pulled DemoIframe out of /help into /dr-ui could we add this functionality as a prop of DemoIframe instead? What are the other use cases for GLWrapper?

@katydecorah
Copy link
Contributor Author

katydecorah commented Aug 12, 2019

@colleenmcginnis Unfortunately, adding DemoIframe to d-rui would only fix part of the problem since we load maps in different ways across our sites. For example:

What are the other use cases for GLWrapper?

We load Mapbox GL JS maps across Help, Vector tiles, Studio Manual, Android, API Playground, and iOS. As described above, we load maps from a few different methods: sometimes DemoIframe other times a component.

A slight alternative

We could pass children instead of a map prop. I think this implementation looks a bit cleaner:

<GLWrapper><DemoIframe src="/help/demos/gl-store-locator/step-five.html" /></GLWrapper>

@colleenmcginnis
Copy link
Contributor

I guess my main concern is that if this wrapper needs to be added at the Markdown file level for every instance, I'm not sure it will always happen. Maybe we should have a dr-ui-based solution for both use cases: iframes and components.

The iframe component would probably be pretty straightforward (very similar to what we're already using in /help). For Mapbox GL JS maps that are being loaded directly via a component, maybe we should create a basic map component that adds the support wrapper by default. Maybe that's a follow-up task, though? Let me know what you're thinking!

@katydecorah
Copy link
Contributor Author

this wrapper needs to be added at the Markdown file level for every instance, I'm not sure it will always happen

I see what you're saying now and agree!

maybe we should create a basic map component that adds the support wrapper by default

I started scoping this out and I think a map component can get pretty complex and would require us to rework all existing GL JS components to use the new component. I think for now, to fix the original issue, adding the GLWrapper around each component is the best option.

Suggested actions:

  1. Keep the GLWrapper component in this PR, we'll use this for map instantiated in a component.
  2. Add the DemoIframe component from Help and use the GLWrapper within the component.

Copy link
Contributor

@colleenmcginnis colleenmcginnis left a comment

Choose a reason for hiding this comment

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

This is a great approach @katydecorah! I left one comment below.

}
}

DemoIframe.propTypes = {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should add another prop here like requiresGl, which defaults to true and adds the GL wrapper, but can be turned off for the few mapbox.js demos we have and the odd demo like Create a timezone finder with Mapbox Tilequery API that doesn't display a GL map. What do you think?

@katydecorah
Copy link
Contributor Author

I think we should add another prop here like requiresGl, which defaults to true and adds the GL wrapper

💯 thanks so much for catching that use case. I just added the new prop.

image
test cases in a browser with GL disabled

Katy DeCorah added 2 commits August 13, 2019 19:29
* master:
  [babel] Add babel config to helpers and plugins, add eslint plugins (#164)
  [Search] Enable babel polyfill on `Search` component (#165)
  [BackToTopButton] use `window.scroll(x-coord, y-coord)`  (#167)
Copy link
Contributor

@colleenmcginnis colleenmcginnis left a comment

Choose a reason for hiding this comment

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

Beautiful 👌 I'm so happy with this solution @katydecorah!

@katydecorah katydecorah merged commit dd0a223 into master Aug 14, 2019
@katydecorah katydecorah deleted the gl-wrapper branch August 14, 2019 17:59
katydecorah pushed a commit that referenced this pull request Aug 14, 2019
* master:
  Add syntax highlighting functions (#172)
  Add `GLWrapper` and `DemoIframe` components. (#166)
katydecorah pushed a commit that referenced this pull request Aug 16, 2019
* master: (24 commits)
  0.20.1
  Prepare 0.20.1
  0.20.0
  Prepare 0.20.0
  Prevent src/components/syntax-highlighters from being adding to pkg
  Update WarningImage in GLWrapper
  Update CHANGELOG.md
  Add syntax highlighting functions (#172)
  Add `GLWrapper` and `DemoIframe` components. (#166)
  compress svg
  update *-image themes and clean up svg, make IE11 friendly, set default `size` & make it a number
  [babel] Add babel config to helpers and plugins, add eslint plugins (#164)
  [Search] Enable babel polyfill on `Search` component (#165)
  [BackToTopButton] use `window.scroll(x-coord, y-coord)`  (#167)
  Create a note "flavor" for new products or features (#162)
  [docs] Update webhook staging urls (#161)
  0.19.3
  Update CHANGELOG.md
  [Note] replace Object.assign with a function (#159)
  0.19.2
  ...
katydecorah pushed a commit that referenced this pull request Sep 3, 2019
* master:
  Add Browser component (#177)
  [test-cases app] create `npm run start-legacy` to test in IE11 (#178)
  Create a video component (#176)
  0.20.1
  Prepare 0.20.1
  0.20.0
  Prepare 0.20.0
  Prevent src/components/syntax-highlighters from being adding to pkg
  Update WarningImage in GLWrapper
  Update CHANGELOG.md
  Add syntax highlighting functions (#172)
  Add `GLWrapper` and `DemoIframe` components. (#166)
  compress svg
  update *-image themes and clean up svg, make IE11 friendly, set default `size` & make it a number
  [babel] Add babel config to helpers and plugins, add eslint plugins (#164)
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