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

Inline-block with conditional IE support #242

Open
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@zsitro

zsitro commented Feb 9, 2014

A snippet that I miss in every project where I use nib. If opacity has place here, then inline-block also should be there

/*
 * Inline-block with conditional IE support.
 */
display(mode, args...)
  if mode == 'inline-block'
    display inline-block args
    if support-for-ie
      zoom 1
      *display inline args
  else
    display mode args
@zsitro

This comment has been minimized.

Show comment
Hide comment
@zsitro

zsitro Mar 4, 2014

@visionmedia does it look good?

zsitro commented Mar 4, 2014

@visionmedia does it look good?

@slang800

This comment has been minimized.

Show comment
Hide comment
@slang800

slang800 Mar 7, 2014

Collaborator

looks like a good idea, except it needs additional tests.

Collaborator

slang800 commented Mar 7, 2014

looks like a good idea, except it needs additional tests.

@zsitro

This comment has been minimized.

Show comment
Hide comment
@zsitro

zsitro Apr 12, 2014

What do you mean @slang800 ? What use-cases are not covered in current test?

zsitro commented Apr 12, 2014

What do you mean @slang800 ? What use-cases are not covered in current test?

@slang800

This comment has been minimized.

Show comment
Hide comment
@slang800

slang800 Apr 12, 2014

Collaborator

Unless I'm totally misunderstanding this PR; We are adding a new mixin that adds properties when we enable support for IE & have display: inline-block. So, there should be a test that makes sure that those properties actually get added.

btw, couldn't the code be refactored into something like this:

display(mode, args...)
  display mode args
  if mode == 'inline-block' && support-for-ie
    // target IE w/ star hack: http://www.ejeliot.com/blog/63
    *zoom 1
    *display inline args

...and should we be putting an * before zoom: 1 too so it only targets IE?

Collaborator

slang800 commented Apr 12, 2014

Unless I'm totally misunderstanding this PR; We are adding a new mixin that adds properties when we enable support for IE & have display: inline-block. So, there should be a test that makes sure that those properties actually get added.

btw, couldn't the code be refactored into something like this:

display(mode, args...)
  display mode args
  if mode == 'inline-block' && support-for-ie
    // target IE w/ star hack: http://www.ejeliot.com/blog/63
    *zoom 1
    *display inline args

...and should we be putting an * before zoom: 1 too so it only targets IE?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment