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

Upgrades code-mirror, browsermob-proxy and mouse-trap #6940

Merged
merged 33 commits into from
Jun 29, 2019
Merged
Changes from 1 commit
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Add Doctring
  • Loading branch information
NishealJ committed Jun 26, 2019
commit 0e1ee0bbc8c062f458965395d439281dd97df19d
3 changes: 2 additions & 1 deletion core/tests/protractor_utils/forms.js
Original file line number Diff line number Diff line change
Expand Up @@ -618,6 +618,7 @@ var CodeMirrorChecker = function(elem) {
* currentLineNumber is the current largest line number processed,
* scrollTo is the number of pixels from the top of the text that
* codemirror should scroll to,
* Pane specifices the codemirror Left or Right Pane which is to be scrolled.
* compareDict is an object whose keys are line numbers and whose values are
* objects corresponding to that line with the following key-value pairs:
* - 'text': the exact string of text expected on that line
Expand All @@ -634,7 +635,7 @@ var CodeMirrorChecker = function(elem) {
// codemirror has loaded. The (2i)th line contains a line number and the
// (2i+1)th line contains the text on that line.
var textArray = text.split('\n');
// Adding an empty line at the last.
// Inserting an empty line at the last of TextArray.
textArray[textArray.length] = '';
Copy link
Member

Choose a reason for hiding this comment

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

Try adding reasons for some "not obvious" code in the comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added PTAL!

Copy link
Contributor

Choose a reason for hiding this comment

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

TextArray-->textArray. Also, I don't think this explanation is sufficient. It does not answer why we need this. Try to explain that as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

@NishealJ, you missed the latter part of the comment:

Also, I don't think this explanation is sufficient. It does not answer why we need this. Try to explain that as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done Added now

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, @NishealJ, you are not actually adding an empty line, right? An empty line would be '\n'. You're adding an empty string (or char).

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, I think this can be improved further:

// in codemirror5's getText() method. Therefore, we need to add an empty string at the end
// of the textArray to match with compareDict.

@DubeySandeep, thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @DubeySandeep & @apb7,
Array matches with dict i.e textArray and compareDict.
To add an empty line in compare dict we would have used a '\n' but as textArray is an array which is already split by '\n' we can just add '' to match with compare dict.
The below screenshot will help you understand the changes in getting a text of empty line from a codemirror5's Pane, you can use console.log(TextArray) to see the changes
Screenshot 2019-06-28 at 12 24 37 PM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The suggested changed looks good to me updated Thanks!

for (var i = 0; i < textArray.length; i += 2) {
var lineNumber = textArray[i];
Expand Down