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

Added more flexible support for custom icons via url #151

Closed
wants to merge 1 commit into from

Conversation

oucil
Copy link

@oucil oucil commented Jun 20, 2013

  • first, in either case "icon" class is applied
  • second step, checks for valid URL (uses regex from "jQuery Validation
    Plugin")
  • for valid URL an inline style is added using "background-image"
  • for class name, previous behaviour is preserved (default)
  • P.S. sorry about all the tab/space removals, blame the editor.

- first, in either case "icon" class is applied
- second step, checks for valid URL (uses regex from "jQuery Validation
Plugin")
- for valid URL an inline style is added using "background-image"
- for class name, previous behaviour is preserved (default)
@rodneyrehm
Copy link
Contributor

Thank you for your PR. I won't merge this one, though…

In Issue #129 I already stated that I'd prefer an iconHook over something like this. An implementor could then create whatever kind of element and css setup they like (including pulling stuff from icon fonts, or image sprites).

@oucil
Copy link
Author

oucil commented Jun 20, 2013

Thanks for considering it :)

Wouldn't this still allow an iconHook since it preserves your default behaviour? Unlike the solution offered in #129 this doesn't add any additional code, it only applies an inline style rather than a class and is completely based on the value supplied.

My reasoning was, I was looking all over your documentation for ways to supply different icons and there isn't anything obvious that didn't involve a lot of work since we have a LOT of icons and unfortunately no use of sprites yet.

I think a lot of users would be in the same position as us, looking for a simple built-in and easily implemented method rather than requiring a lot of extra coding around the edges.

In the end, up to you of course, I just think this makes it more accessible to a larger and potentially less skilled audience.

Cheers!

@oucil oucil closed this Jun 20, 2013
@oucil oucil reopened this Jun 20, 2013
@rodneyrehm
Copy link
Contributor

You're right, maybe we can merge the two ideas.

bbrala added a commit that referenced this pull request Aug 30, 2015
References:
PR#158
Issue #158
Issue #129
Issue #151
Issue #249
@bbrala bbrala closed this Sep 11, 2015
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