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 ex.BoundingBox to have an additional option based constructor #1151

Closed
3 tasks
eonarheim opened this issue May 21, 2019 · 6 comments · Fixed by #1165
Closed
3 tasks

Update ex.BoundingBox to have an additional option based constructor #1151

eonarheim opened this issue May 21, 2019 · 6 comments · Fixed by #1165
Labels
community in-progress This issue is marked in-progress. This label will be removed if there is no activity for 30 days. good first issue good for people new to open source and Excalibur tech debt Label applied to confusing, poorly named, bug prone, or other less than ideal code

Comments

@eonarheim
Copy link
Member

Context

Currently, ex.BoundingBox has a 4 arg parameter list constructor which technically violates the styleguide

Proposal

Create an option back constructor for ex.BoundingBox and preserve the old constructor for backwards compatibility.

export interface BoundingBoxOptions {
   left: number;
   right: number;
   top: number;
   bottom: number;
}

export class BoundingBox {
  ...
  constructor(leftOrOptions: number | BoundingBoxOptions = 0, top: number = 0, right: number = 0, bottom: number = 0) {
       if (leftOrOptions && typeof leftOrOptions === 'object') {
            // initialize properties from leftOrOptions
       } else {
           // initialize from parameter list
       }
  }
  ...
}

const bb = new ex.BoundingBox({
  left: -5,
  right: 5,
  top: 5,
  bottom: -5
});

Acceptance Criteria

  • Existing tests pass
  • New test written for new constructor
  • Changelog entry added under Added section
@eonarheim eonarheim added good first issue good for people new to open source and Excalibur tech debt Label applied to confusing, poorly named, bug prone, or other less than ideal code labels May 21, 2019
@JeTmAn1981
Copy link
Contributor

Is it ok for me to make this change?

I don't have a ton of TS experience, but doing a bit of research it's really too bad it can't allow multiple constructor implementations, that's definitely the way I would go in another language to avoid the awkward use of the union typed leftOrOptions.

@eonarheim
Copy link
Member Author

@JeTmAn1981 all yours!

@eonarheim eonarheim added the community in-progress This issue is marked in-progress. This label will be removed if there is no activity for 30 days. label Jun 9, 2019
@JeTmAn1981
Copy link
Contributor

I've written the new constructor. Regarding tests, the bounding box spec is prefacing all the tests with a call to the old-style constructor:

describe('A Bounding Box', () => {
// left, top, right, bottom
let bb: ex.BoundingBox;
beforeEach(() => {
bb = new ex.BoundingBox(0, 0, 10, 10);
});

Would you like all the subsequent tests to be run for both construction styles (options parameter vs. individually specified coordinate parameters)? If so, is there a way of referencing the existing tests without having to create a new spec file with that code copied over? The only thing that would really need to change is how the constructor is called.

@JeTmAn1981
Copy link
Contributor

Another couple of questions:

  1. I need to test the constructor function on its own. When I run the test suite I get a million errors because something is obviously not working right in the constructor, and that creates a bunch of other failing tests. Buried in all the various errors I'm not seeing anything clues me in to the actual problem; what's wrong with the new constructor. Is there a way to have Karma run just a single test file for that constructor?

  2. Should the constructor tests be included in the main BoundingBox test spec file?

JeTmAn1981 pushed a commit to JeTmAn1981/Excalibur that referenced this issue Jun 10, 2019
…s in an object.

Updated constructor is backwards compatible, still allows users to call it with coordinates as individual parameters.  Main bounding box tests run for boxes constructed using both methods.

Resolves: excaliburjs#1151.
JeTmAn1981 pushed a commit to JeTmAn1981/Excalibur that referenced this issue Jun 10, 2019
…s in an object.

Updated constructor is backwards compatible, still allows users to call it with coordinates as individual parameters.  Main bounding box tests run for boxes constructed using both methods.  Add new interface for specifying the type of the coordinate parameters object.

Resolves: excaliburjs#1151.
@JeTmAn1981
Copy link
Contributor

Ok, I went ahead and created a pull request for what I came up with on this. Let me know how it looks!

eonarheim pushed a commit that referenced this issue Jun 11, 2019
…ter. (#1165)

Updated constructor is backwards compatible, still allows users to call it with coordinates as individual parameters.  Main bounding box tests run for boxes constructed using both methods.  Add new interface for specifying the type of the coordinate parameters object.

Closes #1151
@JeTmAn1981
Copy link
Contributor

Huzzah! So nice to find something small to make a contribution with that gets accepted quickly :)

@jedeen jedeen added this to the 0.24.0 Release milestone Apr 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community in-progress This issue is marked in-progress. This label will be removed if there is no activity for 30 days. good first issue good for people new to open source and Excalibur tech debt Label applied to confusing, poorly named, bug prone, or other less than ideal code
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants