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

Query Overview v2.1.1 #148

Merged
merged 14 commits into from
Jun 7, 2019
Merged

Query Overview v2.1.1 #148

merged 14 commits into from
Jun 7, 2019

Conversation

samussiah
Copy link
Contributor

Issues

Closes #147

@samussiah samussiah added this to the v2.1.1 milestone May 22, 2019
@samussiah samussiah requested a review from pburnsdata May 22, 2019 22:05
@samussiah samussiah added this to In progress in v2.1.1 via automation May 22, 2019
@samussiah
Copy link
Contributor Author

@pburnsdata , in retrospect it was a terrible idea to add unit tests. Took forever, required importing every d3 function explicitly, and was extremely frustrating. You can ignore the explicit import file changes for the most part.

@samussiah samussiah moved this from In progress to Review in progress in v2.1.1 May 23, 2019
@samussiah samussiah self-assigned this May 24, 2019
Copy link
Contributor

@pburnsdata pburnsdata 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! Sorry that the tests got wild - definitely a lot of replaced functions here haha

@@ -25,5 +25,5 @@ export default function onInit() {
removeInvalidControls.call(this);

//Initialize listing.
this.listing.init(this.raw_data);
this.listing.init(this.raw_data, this.listing.test);
Copy link
Contributor

@pburnsdata pburnsdata May 28, 2019

Choose a reason for hiding this comment

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

@samussiah whats going on with this guy?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

webcharts has logic that expects the containing element to have been rendered before it proceeds. but the headless browser we use for testing doesn't actually render the page so there's some logic in init that avoids the rendered logic when running unit tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

gotcha gotcha - makes sense

v2.1.1 automation moved this from Review in progress to Reviewer approved May 28, 2019
@danedexF5 danedexF5 moved this from Reviewer approved to Done in v2.1.1 Jun 3, 2019
@danedexF5 danedexF5 self-requested a review June 3, 2019 19:56
v2.1.1 automation moved this from Done to Reviewer approved Jun 3, 2019
Copy link

@danedexF5 danedexF5 left a comment

Choose a reason for hiding this comment

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

Approved. DESTROY method works.

@danedexF5 danedexF5 moved this from Reviewer approved to Done in v2.1.1 Jun 4, 2019
@danedexF5
Copy link

There actually seems to be an IE issue with destroy. Spencer is looking at it.

@samussiah samussiah moved this from Done to Reviewer approved in v2.1.1 Jun 5, 2019
@samussiah samussiah requested a review from danedexF5 June 5, 2019 19:32
@danedexF5
Copy link

Destroy issue in IE is fixed, all good.

Copy link

@danedexF5 danedexF5 left a comment

Choose a reason for hiding this comment

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

approved

@samussiah samussiah merged commit 2aea898 into master Jun 7, 2019
v2.1.1 automation moved this from Reviewer approved to Done Jun 7, 2019
@samussiah samussiah deleted the dev-v2.1.1 branch June 7, 2019 20:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
v2.1.1
  
Done
Development

Successfully merging this pull request may close these issues.

Add a destroy method.
3 participants