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

Replace OR with AND operator #71

Merged
merged 1 commit into from
Mar 7, 2016

Conversation

tintin1343
Copy link
Contributor

Changed the operators in flexboxIE.js and flexboxOld.js

The issue with the 'OR' operator is that it will create issues in pages which does not have a display property.

Changing the OR operator with AND will do the work.

Closes #70

@tintin1343
Copy link
Contributor Author

@rofrischmann : I do not understand the reason for the tests failure. Could you guide me ?

@robinweser
Copy link
Owner

@tintin1343 Heyhey, just check out the issue again :) I ve posted something about it.
To validate my fix you could add this to the tests:

Current tests

describe('Prefixing display', () => {
  it('should not remove display property', () => {
    expect(new Prefixer({ userAgent: MSIE10 }).prefix({
      display: 'block'
    })).to.eql({ display: 'block' })
  })
})

New tests

describe('Prefixing display', () => {
  it('should not remove display property', () => {
    expect(new Prefixer({ userAgent: MSIE10 }).prefix({
      display: 'block'
    })).to.eql({ display: 'block' })
  })
  it('should not throw if display is null or undefined', () => {
    expect(new Prefixer({ userAgent: Chrome45 }).prefix({
      display: null
    })).to.eql({ display: null })
    expect(new Prefixer({ userAgent: Chrome45 }).prefix({
      display: undefined
    })).to.eql({ display: undefined })
  })
})

@tintin1343
Copy link
Contributor Author

@rofrischmann : Ok. I saw your comment on the issue. Let me make the necessary changes and edit my pull request.

Changed the operators in flexboxIE.js and flexboxOld.js
robinweser pushed a commit that referenced this pull request Mar 7, 2016
Replace OR with AND operator
@robinweser robinweser merged commit 01dce75 into robinweser:master Mar 7, 2016
@robinweser
Copy link
Owner

Alright thanks a lot! Tests seem to pass now :P The only reason for the failed status is actually an issue with codeclimate which I am not sure how to fix right now.

@tintin1343
Copy link
Contributor Author

@rofrischmann : Cool. I was wondering why that was happening when my npm test passed. You are welcome.

@necolas
Copy link

necolas commented Mar 7, 2016

Does the prefix-all package also need part of this patch?

@robinweser
Copy link
Owner

@necolas Actually it should work for your usecase as far as I know you do not have null or undefined values right? Still I will add this right away.

@robinweser
Copy link
Owner

@necolas https://github.com/rofrischmann/inline-style-prefix-all/releases/tag/1.0.2 here we go. I actually could remove this display check at all as we already use the full flex plugin :P

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.

flexboxIE and flexboxOld fail to check in case of value is null
3 participants