Address JS error in visualizer when displaying the dependency graph #2042
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@amarco90, thanks for your PR! By analyzing the history of the files in this pull request, we identified @daveFNbuck, @nmb10 and @riga to be potential reviewers. |
|
It wasn't my commit, but I recently stumbled upon this as well. I added an inline comment. |
riga
reviewed
Feb 25, 2017
| @@ -504,9 +504,6 @@ function visualiserApp(luigi) { | |||
|
|
|||
| $('input[name=vis-type]').on('change', function () { | |||
| changeState('visType', $(this).val()); | |||
|
|
|||
| initVisualisation(visType); | |||
There was a problem hiding this comment.
I think both initVisualization and updateVisType should be invoked here. Maybe this way:
var visType = $(this).val();
changeState('visType', visType);
initVisualisation(visType);
updateVisType(visType);There was a problem hiding this comment.
That would work but then updateVisType would be called twice since processHashChange also calls it.
|
Is this ready to be merged? |
|
I think it is. Do you agree @riga ? |
|
Thanks for this patch @amarco90! |
|
@amarco90 Sorry I'm a bit late, but yes, your changes work for me. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
In the current version of the visualizer an error raises when switching to the Dependency Graph tab. To reproduce it just load the visualizer page, open a javascript console and go to the Dependency Graph tab.
The error is happening because the variable
visTypeis not defined. The variablefragmentQuery.visTypein the function call in the next line is not defined either.To address the issue I removed the function calls that were raising an error because of undeclared variables. The function calls were anyway not required, because line 506 modifies the state of the address bar. An event listening to changes on the address bar takes care of updating the visualization graph.
Motivation and Context
I think the error was introduced in this commit.
Have you tested this? If so, how?
I tested the fix locally, now the visualizer does not produce errors in the dependency graph tab and still works normally.