-
-
Notifications
You must be signed in to change notification settings - Fork 30
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
Added typed properties support and properties for TileMap and Layer #3
Conversation
lib/src/tile_map_parser.dart
Outdated
final a = value.substring(1, 3); | ||
final rgb = value.substring(3); | ||
|
||
map[name] = '#' + rgb + a; |
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'm not sure what this color
type entails but on Flame/Flutter we often use colors as ARGB, not RGBA. (eg Color class constructor)
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.
Is there method to check if environment is web? And then optional parse to RGBA. In web ARGB is useless.
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.
hum I thought dart would standardize that... let's leave this as is but please add a todo comment, it's clearly an edge case and we can study this later. but I prefer to follow dart conventions, flutter-web should be translating whatever it needs.
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 have some comments!
@luanpotter I added commit with changes. |
lib/src/layer.dart
Outdated
|
||
properties = TileMapParser._parseProperties( | ||
TileMapParser._getPropertyNodes(element), | ||
); |
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.
it seems it would be useful to have a TileMapParser._parsePropertiesFromXML method to be used in 3 places
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.
LGTM with one nit
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.
We need to add an entry to the CHANGELOG and finish these missing comments by @luanpotter
No description provided.