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 ability to customize html background color #386

Merged

Conversation

zikolach
Copy link
Contributor

@zikolach zikolach commented Jun 20, 2022

It is useful while developing a game with dark background (to avoid showing white empty HTML page on browser reloads).

@davesmith00000 davesmith00000 requested a review from a team June 20, 2022 08:36
@zikolach zikolach marked this pull request as ready for review June 20, 2022 17:23
@zikolach
Copy link
Contributor Author

@davesmith00000 could you please have a look? I made changes according to your suggestions. Switching white to black really hurts the eyes when reloading page during development 👓

@davesmith00000
Copy link
Member

Thanks, will do! I can't review this properly today but I'll have a look soon!

The thing that stands out after a quick glance is the default "white" value on the case class. I don't normally do that - but it might be ok when I have time to look at it properly in context.

Copy link
Member

@davesmith00000 davesmith00000 left a comment

Choose a reason for hiding this comment

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

Looks good broadly, thank you for the work! I've left a couple of comments for you to review please.

@@ -2,7 +2,7 @@ package indigoplugin.templates

object HtmlTemplate {

def template(title: String, showCursor: Boolean, scriptName: String): String =
def template(title: String, showCursor: Boolean, scriptName: String, backgroundColor: String): String =
Copy link
Member

Choose a reason for hiding this comment

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

I feel like this methods signature should now be:

def template(scriptName: String, templateOptions: TemplateOptions): String

Otherwise we're just expanding the fields of the case class into function arguments?

@@ -6,7 +6,8 @@ final case class TemplateOptions(
title: String,
showCursor: Boolean,
scriptPathBase: Path,
gameAssetsDirectoryPath: Path
gameAssetsDirectoryPath: Path,
backgroundColor: String = "white"
Copy link
Member

Choose a reason for hiding this comment

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

Do we need the default? Can it be removed? I suspect it's unnecessary? SBT will default to white and Mill forces you to provide a value.

val showCursor: SettingKey[Boolean] = settingKey[Boolean]("Show the cursor? True by default.")
val title: SettingKey[String] = settingKey[String]("Title of your game. Defaults to 'Made with Indigo'.")
val backgroundColor: SettingKey[String] =
settingKey[String]("HTML page background color CSS property. Defaults to 'initial'.")
Copy link
Member

Choose a reason for hiding this comment

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

Description says 'initial' but line 48 sets it to 'white'.

@davesmith00000 davesmith00000 merged commit 54c7ded into PurpleKingdomGames:main Jul 14, 2022
@zikolach zikolach deleted the feature/background-color branch July 14, 2022 15:18
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