-
Notifications
You must be signed in to change notification settings - Fork 450
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
FlxBasePreloader: improve the sitelock failure notice #1994
Conversation
flixel/system/FlxBasePreloader.hx
Outdated
} | ||
|
||
private function createSiteLockFailureBackground(innerColor:UInt, outerColor:UInt):Shape | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code uses a mix of tabs and spaces. Technically haxe-checkstyle / CodeClimate should catch this, but I've had issues with that in the past IIRC... :/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it's because of HaxeCheckstyle/haxe-checkstyle#278.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry about this. VSCode normally does the right thing by matching the formatting of the file you are editing. However, I use spaces in my own code, so any new files that I write will use spaces instead of tabs. Since I had to do a big copy-paste job for this one, the spaces I normally used leaked in here. I thought I caught that and normalized it, but I think that may have only been for a few lines here and there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good change, but please add indication on how users can override default values.
flixel/system/FlxBasePreloader.hx
Outdated
*/ | ||
public static inline var LOCAL:String = #if flash "local" #else "localhost" #end; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the extra lines?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extra lines? Are you referring to the whitespace normalization? If so, there should not be any "extra" lines. I will of course double-check once I get this branch rebased to address the changes you requested below as well as the conflicts due to the PR's old age.
|
||
/** | ||
* The title text to display on the sitelock failure screen. | ||
* NOTE: This string should be reviewed for accuracy and may need to be localized. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Provide hint on how user can override this text, ex:
* To override this variable, create a new preloader class that extends FlxBasePreloader, and override the default value in new function before you call super();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added clarification on how to customize as well as a providing a simple example.
|
||
/** | ||
* The body text to display on the sitelock failure screen. | ||
* NOTE: This string should be reviewed for accuracy and may need to be localized. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here.
74617c2
to
fb461c7
Compare
@Gama11 any reservations about this change? |
Can we add More generally, this seems like a lot of effort for a fairly niche / little-used feature, but hey, it's already here now, and it looks nice. :) |
flixel/system/FlxBasePreloader.hx
Outdated
addChild(createSiteLockFailureText(30)); | ||
} | ||
|
||
private function createSiteLockFailureBackground(innerColor:UInt, outerColor:UInt):Shape |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason not to use FlxColor
for colors?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure. I'm fairly certain FlxColor
would work. I most likely wrote it this way since the underlying graphics APIs expect UInt
.
@Gama11 What do I need to put in for a |
4.3.0 actually, since that will be the next release. Example: https://github.com/HaxeFlixel/flixel/blob/dev/flixel/FlxCamera.hx#L1529 |
Improved the visuals of the failure notice. Revised the default failure notice text. Enabled easy customization of the failure notice text and formatting. Provide example for customizing title/body text. Added since tags for 4.3.0. Changed UInt to FlxColor for color-based arguments. Fix Travis build break against Haxe 3.2.0. An Array<Int> is not being converted to a Vector<Float> when compiled with the older version of Haxe. Making the first element an explicit floating-point value will force the type to Array<Float> which, hopefully, should convert as expected.
0d091bf
to
593d33d
Compare
Okay, that should do it. Thanks, @Gama11 and @gamedevsam for all the feedback and help. |
This doesn't really affect #2055 (for better or worse), so I'll go ahead and merge it. Definitely looks a lot nicer than before, thanks! |
The existing sitelock failure notice is, well, a little lackluster. Unfortunately, the sitelock functionality within the
FlxBasePreloader
class is not as easily modified, requiring the programmer to duplicate parts of the logic not dealing with the visuals.I decided the improvements I made for my own uses should be shared with the world, so here is a PR for including an updated version of the sitelock failure notice. And as pictures are nice, here is how the new default failure notice looks:
As the underlying logic has been reorganized to address the issues mentioned above and several helper functions have been added to aid in the creation of the visual elements, here is a code snippet showing a sample customization in action: