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

Add set font icon #2856

Merged
merged 2 commits into from Jul 9, 2019

Conversation

@ThomasH99
Copy link
Contributor

commented Jun 26, 2019

Hi, I made an attempt to add support for Font Icons (in addition to built-in support for Material Icons). Primarily by generalizing the support you already had for Material icons.

As discussed recently in the Blog: https://www.codenameone.com/blog/using-icon-fonts-such-as-fontello.html

ThomasH99 added 2 commits Jun 12, 2019
Merge pull request #19 from codenameone/master
Update from CN1 master
Added support for adding icons from any icon font in a similar way as…
… material icons are supported. Quite a few changes to add all the same methods as for material icons - feel free to double-check if I missed some. Also added the support to Labels (but not Command or Button).
@codenameone

This comment has been minimized.

Copy link
Owner

commented Jun 27, 2019

You can't remove the setMaterialIcon API's. These are used all over the place. You can't just rename them to add your functionality. This breaks compatibility which is something we don't do.

You can add another method and/or enhance functionality. E.g. setMaterialIcon can use a font icon if it's an icon font is explicitly defined for the component.

Also notice that the set methods in the ImageFont class predated the API for material icon in label. Had that API existed back then they would have used that API. So when you create a new API you should probably use the API in label and add an API to ImageFont just as a delegate to that API.

@ThomasH99

This comment has been minimized.

Copy link
Contributor Author

commented Jul 3, 2019

I think you didn't review the changes in detail. I did not remove or change any of the existing APIs, I only reshuffled some code from the existing material icon methods to the set font icon methods to avoid duplication. Let me know if I missed something or misunderstood your answer.

@codenameone codenameone merged commit d586080 into codenameone:master Jul 9, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@codenameone

This comment has been minimized.

Copy link
Owner

commented Jul 9, 2019

Thanks!

You're right. I got stumped by a merge highlight that looked wrong and missed the code that fixed it below.

@ThomasH99

This comment has been minimized.

Copy link
Contributor Author

commented Jul 11, 2019

Happy to be able to contribute :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.