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

HTML5FrontEnd: Add platform and mobile detection #1897

Merged
merged 8 commits into from
Aug 4, 2016
Merged

HTML5FrontEnd: Add platform and mobile detection #1897

merged 8 commits into from
Aug 4, 2016

Conversation

DleanJeans
Copy link
Contributor

@DleanJeans DleanJeans commented Aug 3, 2016

I added platform and isMobile variables to FlxG.html5.
Tested on Windows and Android. Not sure about the others. Here's a test (https://github.com/DleanJeans/flixel-html5-platform-test).

@@ -59,6 +61,49 @@ class HTML5FrontEnd
{
return Browser.window.innerHeight;
}

private inline function get_platform():FlxPlatform
Copy link
Member

Choose a reason for hiding this comment

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

This function is way to long to be a good candidate for inline.

Copy link
Member

Choose a reason for hiding this comment

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

We could probably just run this entire thing once in new() and cache the result? It's not like it's gonna change at runtime, right? :)

@Gama11 Gama11 added this to the 4.2.0 milestone Aug 3, 2016
private function new()
{
browser = getBrowser();
browserWidth = getBrowserWidth();
Copy link
Member

Choose a reason for hiding this comment

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

Browser width and height could change at runtime, so I don't think caching those is a good idea.

WINDOWS;
LINUX;
UNIX;
MAC;
Copy link
Member

Choose a reason for hiding this comment

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

Hm... MAC isn't used anywhere?

@DleanJeans
Copy link
Contributor Author

I think we really need more people to test this

@DleanJeans
Copy link
Contributor Author

Sorry for my ignorance. Here's my reference. So you're saying it should be removed?

@Gama11
Copy link
Member

Gama11 commented Aug 4, 2016

I'm not sure, I was mostly asking out of ignorance as well. ;)

@DleanJeans
Copy link
Contributor Author

Maybe it's for some kind of Unix-based OS which is probably rarely used by our users. So I guess it should be removed anyway.

Replace iOS with iPhone/Pad/Pod
Add missing Mac (oops)
Remove Unix
@Gama11
Copy link
Member

Gama11 commented Aug 4, 2016

It feels a bit strange to have IPOD / IPAD / IPHONE in the enum directly, since everything else in there is an OS... It might make sense to continue having IOS as an enum value, but with an enum constructor argument that is another enum (which is then either IPOD, IPAD or IPHONE)?

@Gama11 Gama11 merged commit 7136c47 into HaxeFlixel:dev Aug 4, 2016
@Gama11
Copy link
Member

Gama11 commented Aug 4, 2016

Thanks! :)

Aurel300 pushed a commit to larsiusprime/haxeflixel that referenced this pull request Apr 18, 2018
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.

2 participants