Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Render icons in a span tag when using a class name #964

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
3 participants
Member

nicolasbadia commented Mar 15, 2013

More information about why this change: #963

This pull request include:

  • replacement of icons img tags by span tags when using a class name
  • update of the CSS rules to match that change
  • wrapping of the CSS rules of the radio view into sc-radio-view
  • icon margin-right fix for the button views
  • icon positioning fix of 18px button views
  • rename of the label classname of the checkbox view from label to sc-button-label for consistency.

@nicolasbadia nicolasbadia referenced this pull request in sproutcore/showcase Mar 15, 2013

Merged

Fix few rules to match icon tags changes #2

Owner

dcporter commented Mar 15, 2013

I'm curious why you changed plans from div to span? I believe div is used elsewhere for this purpose and I wonder if there's any difference.

Member

nicolasbadia commented Mar 15, 2013

span and div shouldn't make any difference since all the icons are displayed inline-block.
I don't really have a reason of why I choose span over div but if div make more sense to you, I can change this.

Owner

dcporter commented Mar 15, 2013

I'm generally mistrustful of span, but I sure couldn't tell you why, beyond that I've run into random issues that were fixed by swapping it back out with div. If it works then I suppose I'm just being codgery.

Owner

publickeating commented Mar 15, 2013

Because we have to update the framework stylesheets and the Showcase app stylesheets, shows that this is going to break people's apps, so it will have to wait for 2.0 or else be done in such a way that is non-breaking for current apps.

Marking it for 2.0.

@nicolasbadia nicolasbadia added a commit to nicolasbadia/sproutcore-wysiwyg that referenced this pull request Mar 24, 2013

@nicolasbadia nicolasbadia Fix css rule to match futur icon tags changes 8c28339
Member

nicolasbadia commented Jun 9, 2014

Closed in favor of #1260

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