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

Refactor guide line layer - Pass 1 #2666

Merged
merged 6 commits into from
Aug 24, 2015
Merged

Refactor guide line layer - Pass 1 #2666

merged 6 commits into from
Aug 24, 2015

Conversation

aicioara
Copy link
Contributor

Part of #2623

I will be doing 2 passes because I will get more context (and better sense of best practices) by refactoring other files. However, please flag anything that seems unusual.

As part of this:

  • Test semantics: hierarchy, naming, testing, but no coverage check
  • Applied best practices from the wiki page
  • Factored out common code into beforeEach() clauses
  • No nested beforeEach()es
  • Remade hierarchy as shown bellow
    screen shot 2015-08-24 at 2 57 40 pm

assert.strictEqual(gll.width(), SVG_WIDTH, "accepted all offered width");
assert.strictEqual(gll.height(), SVG_HEIGHT, "accepted all offered height");
svg.remove();
describe("Interactive Components", () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

GuideLineLayer isn't interactive, since it doesn't include an Interaction, so I don't think it belongs in this describe().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then let's find an umbrella for all of them. Layer Components ? It should include

  • dragBoxLayerTests.ts
  • guidelinesLayerTests.ts
  • selectionBoxLayerTests.ts
  • xDragBoxLayerTests.ts
  • yDragBoxLayerTests.ts

@aicioara aicioara removed the -1 label Aug 24, 2015
@aicioara aicioara assigned jtlan and unassigned aicioara Aug 24, 2015
describe("Rendering (vertical)", () => {
const SVG_WIDTH = 400;
const SVG_HEIGHT = 300;
const GUIDE_LINE_CLASS = "." + "guide-line";
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we look into setting up a TSLint rule for having const on variables with ALL_CAPS naming?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is none.

@jtlan jtlan assigned aicioara and unassigned jtlan Aug 24, 2015
@aicioara
Copy link
Contributor Author

Opened palantir/tslint#604

@aicioara
Copy link
Contributor Author

I will convert back to let until we have an actual rule.

@aicioara aicioara assigned jtlan and unassigned aicioara Aug 24, 2015
@jtlan jtlan added the +1 label Aug 24, 2015
@jtlan jtlan assigned aicioara and unassigned jtlan Aug 24, 2015
aicioara added a commit that referenced this pull request Aug 24, 2015
@aicioara aicioara merged commit 9f20c41 into develop Aug 24, 2015
@aicioara aicioara deleted the refactorGuideLineLayer branch August 24, 2015 23:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants