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

fix(Modal): fix errors with SSR #2260

Merged
merged 2 commits into from
Oct 29, 2017
Merged

Conversation

adam-26
Copy link
Contributor

@adam-26 adam-26 commented Oct 26, 2017

Resolve Issue #2259

@codecov-io
Copy link

codecov-io commented Oct 26, 2017

Codecov Report

Merging #2260 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2260      +/-   ##
==========================================
+ Coverage   99.73%   99.73%   +<.01%     
==========================================
  Files         151      151              
  Lines        2624     2625       +1     
==========================================
+ Hits         2617     2618       +1     
  Misses          7        7
Impacted Files Coverage Δ
src/addons/Responsive/Responsive.js 100% <100%> (ø) ⬆️
src/modules/Sticky/Sticky.js 100% <100%> (ø) ⬆️
src/modules/Search/Search.js 99.46% <100%> (ø) ⬆️
src/addons/Portal/Portal.js 100% <100%> (ø) ⬆️
src/modules/Dimmer/Dimmer.js 100% <100%> (ø) ⬆️
src/modules/Popup/Popup.js 100% <100%> (ø) ⬆️
src/modules/Modal/Modal.js 100% <100%> (ø) ⬆️
src/behaviors/Visibility/Visibility.js 100% <100%> (ø) ⬆️

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 01b02f7...c26d54e. Read the comment docs.

@adam-26
Copy link
Contributor Author

adam-26 commented Oct 26, 2017

@layershifter PR submitted for #2259

@@ -155,13 +158,16 @@ class Modal extends Component {
static Description = ModalDescription
static Actions = ModalActions

// expose for testing
static isBrowser = isBrowser
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason to not import isBrowser in tests instead?

Copy link
Member

@levithomason levithomason Oct 29, 2017

Choose a reason for hiding this comment

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

Oh, I see. I'd like to have a way to spoof isBrowser that is not dependant on the Modal. One idea is to convert isBrowser to a function. This way, we can set a flag to override it:

const isBrowser = () => !_.isNil(isBrowser, 'override')
  ? isBrowser.override
  : hasDocument && hasWindow

export default isBrowser

Then we can do something similar to what is being done now, except with isBrowser rather than Modal:

before(() => {
  isBrowser.override = false
  // isBrowser() === false
})

after(() => {
  isBrowser.override = null
  // isBrowser() === <appropriate value>
})

Allow isBrowser to be overridden for SSR tests
@layershifter layershifter changed the title fix: Modal SSR fix(Modal): fix errors with SSR Oct 29, 2017
@levithomason levithomason merged commit 454daaa into Semantic-Org:master Oct 29, 2017
@levithomason
Copy link
Member

Thanks!

@levithomason
Copy link
Member

Released in [email protected]

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

Successfully merging this pull request may close these issues.

3 participants