Skip to content

Replace OR with AND operator#71

Merged
robinweser merged 1 commit intorobinweser:masterfrom
tintin1343:flexbox-fix
Mar 7, 2016
Merged

Replace OR with AND operator#71
robinweser merged 1 commit intorobinweser:masterfrom
tintin1343:flexbox-fix

Conversation

@tintin1343
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

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

@robinweser
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor Author

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

@necolas
Copy link
Copy Markdown

necolas commented Mar 7, 2016

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

@robinweser
Copy link
Copy Markdown
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
Copy Markdown
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