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

Add IE11 compatibility meta tag #2650

Merged
merged 2 commits into from
Jan 4, 2018
Merged

Conversation

mynnx
Copy link
Contributor

@mynnx mynnx commented Jan 4, 2018

Issue:

What I did

I could get IE11 to render the Storybook, but it would not render the iframe view. It kept defaulting to older document modes which were too outdated to render our JS.

The only way I could reliably get IE11 to render the iframe view was to add the following meta tag, which is present in the Storybook template:

<meta content="IE=edge" http-equiv="X-UA-Compatible" />

I'm not well-versed in how this works; my reading suggests that an HTTP header is usually preferred but we're working with webpack-dev-server and don't have a ton of control over that.

If anyone has suggestions about how this should work, I'm open to hearing them.

How to test

Open the iframe view in IE11 with and without the meta tag; observe that it does not render without.

Is this testable with jest or storyshots?
Not unless we use Browserstack or Saucelabs

Does this need a new example in the kitchen sink apps?

No

Does this need an update to the documentation?

No

@ndelangen
Copy link
Member

Adding the header on static builds will be super annoying too, so this is perfect actually!

Thank you for caring @mynnx !

@ndelangen
Copy link
Member

Could I request you also perform this change for the Vue and Angular apps?

@storybookjs storybookjs deleted a comment from codecov bot Jan 4, 2018
@codecov
Copy link

codecov bot commented Jan 4, 2018

Codecov Report

Merging #2650 into master will increase coverage by 0.06%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #2650      +/-   ##
=========================================
+ Coverage   34.34%   34.4%   +0.06%     
=========================================
  Files         389     389              
  Lines        8750    8777      +27     
  Branches      914    1131     +217     
=========================================
+ Hits         3005    3020      +15     
+ Misses       5129    5032      -97     
- Partials      616     725     +109
Impacted Files Coverage Δ
app/vue/src/server/utils.js 44.11% <0%> (-9.46%) ⬇️
addons/info/src/components/Story.js 88.88% <0%> (ø) ⬆️
addons/info/src/components/makeTableComponent.js 67.46% <0%> (ø) ⬆️
lib/ui/src/modules/api/index.js 0% <0%> (ø) ⬆️
...tive/src/preview/components/StoryListView/index.js 0% <0%> (ø) ⬆️
addons/a11y/src/components/Report/Info.js 0% <0%> (ø) ⬆️
addons/knobs/src/components/types/Boolean.js 11.62% <0%> (ø) ⬆️
addons/jest/src/components/PanelTitle.js 0% <0%> (ø) ⬆️
addons/links/src/react/components/link.js 78.78% <0%> (ø) ⬆️
.../src/modules/ui/components/stories_panel/header.js 29.62% <0%> (ø) ⬆️
... and 58 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fe409eb...479f1a5. Read the comment docs.

@mynnx
Copy link
Contributor Author

mynnx commented Jan 4, 2018

@ndelangen Thanks for the quick response! I tried following the instructions at https://github.com/storybooks/storybook/blob/master/docs/src/pages/configurations/add-custom-head-tags/index.md to add this tag, but found that it was not injected. Can you verify that this doc page is still accurate and that the functionality still works?

@ndelangen
Copy link
Member

Epic! Merge when ready

@mynnx
Copy link
Contributor Author

mynnx commented Jan 4, 2018

@ndelangen I can't merge without write access and it appears that CI failed. Anything you'd like me to do?

@ndelangen
Copy link
Member

ndelangen commented Jan 4, 2018

I'm looking into it, looks like an outage at circleCI. I have re-scheduled the CI.

I leave the "Merge when ready" remark, because I usually don't wait around for the CI to approve, and browse elsewhere. With that comment other maintainers know it was my intention to merge. Or future-me 😊.

@ndelangen ndelangen merged commit 35d2636 into storybookjs:master Jan 4, 2018
@ndelangen ndelangen changed the title Add IE compatibility meta tag Add IE11 compatibility meta tag Jan 4, 2018
@Hypnosphi Hypnosphi added the patch:yes Bugfix & documentation PR that need to be picked to main branch label Jan 4, 2018
shilman pushed a commit that referenced this pull request Jan 7, 2018
Add IE compatibility meta tag
@Hypnosphi Hypnosphi added the patch:done Patch/release PRs already cherry-picked to main/release branch label Mar 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug patch:done Patch/release PRs already cherry-picked to main/release branch patch:yes Bugfix & documentation PR that need to be picked to main branch ui
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants