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

Update RoomVisual.poly to also accept positions #113

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

Dessix
Copy link
Collaborator

@Dessix Dessix commented Jul 22, 2017

From the documentation:

points | array | An array of points.
Every item should be either an array with 2 numbers (i.e. [10,15]), or a RoomPosition object.

@Dessix Dessix requested review from kotarou and anisoptera July 22, 2017 22:15
Copy link
Member

@kotarou kotarou left a comment

Choose a reason for hiding this comment

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

Verified that the PR works.

This misses one example of poly usage, which is also the example given in the screeps documentation - using an array of objects that contain x and y properties (e.g. a path).

Recommend changing to poly(points: Array<{x:number, y:number} | [number, number] | RoomPosition>, style?: PolyStyle): RoomVisual; or equivalent.

Dessix added 2 commits July 22, 2017 19:24
Updated tsconfig.json newLine to meet new Typescript compiler casing requirements
@Dessix
Copy link
Collaborator Author

Dessix commented Jul 23, 2017

The changes I've pushed allow objects meeting the { x: number, y: number } structural interface to be used in place of RoomPosition instances. I've tested these as working. This also changes RoomVisual's constructor to the Typescript standard es6 declaration style, wherein the constructor and instance types are separate interfaces, and provides separate subtypes for differentiating the availability of roomName depending on whether or not the "global" constructor was used.

Copy link
Member

@kotarou kotarou left a comment

Choose a reason for hiding this comment

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

Visually looks really good to me. Will explicitly test on prod scripts and check soon.

@thaelina
Copy link
Contributor

thaelina commented Oct 3, 2017

I tested these changes and was unable to declare a RoomVisual
Error:(39, 19) TS2693:'RoomVisual' only refers to a type, but is being used as a value here.
let vis = new RoomVisual();

Need to add declare const RoomVisual: RoomVisualConstructor;

I know this has been languishing here, but hopefully we can get this repo updated and such

@Dessix
Copy link
Collaborator Author

Dessix commented Oct 4, 2017

Added constructor object instance for RoomVisual as per @thaelina's suggestion and rebuilt.

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