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

Rework #45

Merged
merged 13 commits into from
Dec 31, 2021
Merged

Rework #45

merged 13 commits into from
Dec 31, 2021

Conversation

DotWith
Copy link
Contributor

@DotWith DotWith commented Dec 29, 2021

Major Changes

Added hxformat.
getFont returns name and size. Removed getFontSize.

TODO

@larsiusprime
Copy link
Owner

I can merge this when you're done, but since it looks like it has breaking changes we'll have to bump the version number a full point when it's done.

What's the main thing you're trying to achieve here?

@DotWith
Copy link
Contributor Author

DotWith commented Dec 29, 2021

I can merge this when you're done, but since it looks like it has breaking changes we'll have to bump the version number a full point when it's done.

What's the main thing you're trying to achieve here?

I'm attempting to make the library more easier to use.

@larsiusprime
Copy link
Owner

Cool! What are the chief improvements in this PR?

@DotWith
Copy link
Contributor Author

DotWith commented Dec 29, 2021

Cool! What are the chief improvements in this PR?

I need to add backwards compatibly with the old init function and getFontSize. I mark them as deprecated.

Init now uses params

tongue.init({
        locale: "en-US",
        finishedCallback: onLoaded
    });

getFontSize has been removed and instead replaced with this.

var oldfont = "verdana";
var oldsize = 12;
var newfont = tongue.getFont(oldfont, oldsize);
trace(newfont.name); // returns "comicsans"
trace(newfont.size); // returns 14

@larsiusprime
Copy link
Owner

Ah, that's much more convenient. Looks good, thanks for doing this!

@DotWith
Copy link
Contributor Author

DotWith commented Dec 29, 2021

Ah, that's much more convenient. Looks good, thanks for doing this!

I was hoping to add other property's like position, etc. To the FontData, but I haven't found a reason to add it so far.

<?xml version="1.0" encoding="utf-8" ?>
<data>
	<font value="verdana" replace="arial">
            <x value="13" replace="199"/>
            <y value="18" replace="456"/>
	</font>
</data>

@DotWith
Copy link
Contributor Author

DotWith commented Dec 29, 2021

I'll add xml support into another pull request.

@DotWith DotWith marked this pull request as ready for review December 29, 2021 19:45
@DotWith
Copy link
Contributor Author

DotWith commented Dec 29, 2021

@larsiusprime You shouldn't have to bump up the version to a full point anymore. Now that backwards compatibly has been added. Once you do merge, please update the CHANGELOG.md.

@larsiusprime
Copy link
Owner

Cool, let me know when you're done!

@DotWith
Copy link
Contributor Author

DotWith commented Dec 30, 2021

@larsiusprime I'm done. I've tested the example project. This pr will close #40 and #43.

Copy link
Collaborator

@EliteMasterEric EliteMasterEric left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@larsiusprime For reference, I collaborated with DotWith on this, and made sure that the improvements they made were backwards compatible, so they should work without interfering with existing projects.

@@ -107,101 +136,123 @@ class FireTongue
* @param getDirectoryContents (optional) custom function to list the contents of a directory
* @param forceCase (optional) what case to force for flags in CSV/TSV files, and in the get() function -- default is to force uppercase.
*/
public function new(?framework:Framework, ?checkFile:String->Bool, ?getText:String->String, ?getDirectoryContents:String->Array<String>, ?forceCase:Case=Case.Upper)
public function new(?framework:Framework, ?checkFile:String->Bool, ?getText:String->String, ?getDirectoryContents:String->Array<String>,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice if this function could be reworked to use another parameter object, including a customFramework class instead of individual functions, but that should probably be postponed to the next point where a major version is deemed necessary.

@larsiusprime
Copy link
Owner

@larsiusprime For reference, I collaborated with DotWith on this, and made sure that the improvements they made were backwards compatible, so they should work without interfering with existing projects.

Happy to make you a collaborator on this repo too, but I'll try to rule on this PR by end of day nonetheless.

Copy link
Owner

@larsiusprime larsiusprime left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good!

@larsiusprime larsiusprime merged commit c78e5ca into larsiusprime:dev Dec 31, 2021
@DigiEggz DigiEggz mentioned this pull request Jan 11, 2024
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