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

Adding an example for the String.substring() method #1074

Merged
merged 10 commits into from
Aug 15, 2018
Merged

Adding an example for the String.substring() method #1074

merged 10 commits into from
Aug 15, 2018

Conversation

irenesmith
Copy link
Contributor

No description provided.

@wbamberg wbamberg self-requested a review August 4, 2018 04:37
Copy link
Contributor

@wbamberg wbamberg left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

I had a couple of comments on this example, and also many of the comments from #1073 also apply to this one.

console.log(str.substring(2,str.length)); 'zilla'
</code>
</pre>

Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove this line containing just spaces, ad all files should end with a return character (that's what the
screen shot 2018-08-10 at 12 22 07 pm is telling you).

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

<pre>
<code id="static-js">const str = 'Mozilla';
console.log(str.substring(1,2)); // 'oz'
console.log(str.substring(2,str.length)); 'zilla'
Copy link
Contributor

Choose a reason for hiding this comment

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

There's no point doing this: str.substring(2,str.length), because it's just the same as str.substring(2).

In fact let's use str.substring(2) instead on this line, then we can illustrate the optional parameter.

Copy link
Contributor

@wbamberg wbamberg left a comment

Choose a reason for hiding this comment

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

Thanks for the updates! There are still a couple of issues left in this one.

console.log(str.substring(2));
// expected output "zilla"
</code>
</pre>
Copy link
Contributor

Choose a reason for hiding this comment

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

This line still seems to end without a newline.

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 should be one now.

<code id="static-js">var str = 'Mozilla';

console.log(str.substring(1, 2));
// expected output "oz"
Copy link
Contributor

Choose a reason for hiding this comment

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

This actually returns "o" not "oz". Sorry, should have caught this last time round.

Also, sorry to be super-nitpicky, but the format is:

// expected output: thing

Note the colon. Sorry to be so nitpicky, but it's worth being exact about this I think.

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 should have caught "oz" too. I fixed the code so it produces the intended result and also fixed the comments.

<code id="static-js">var str = 'Mozilla';

console.log(str.substring(1, 2));
// expected output: "oz"
Copy link
Contributor

Choose a reason for hiding this comment

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

This still seems to be wrong. It should be either (1, 3) or expected output: "o"

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 would have sworn that I made this change already but I've fixed it and pushed again.

Copy link
Contributor

@wbamberg wbamberg 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, thanks @irenesmith !

@wbamberg wbamberg merged commit d6a133e into mdn:master Aug 15, 2018
wbamberg pushed a commit to wbamberg/interactive-examples that referenced this pull request Aug 31, 2018
* upstream/master:
  Add window.ieConfig.origin for the target origin (mdn#1112)
  Improve example code (mdn#1111)
  Adding an example for the String.substring() method (mdn#1074)
  Adding an example for String.slice() (mdn#1081)
  Adding an example of String.split() (mdn#1071)
  Adding an example for the String.substr() method (mdn#1073)
  chore(deps): update dependency jest to v23.5.0 (mdn#1109)
  chore(deps): update dependency puppeteer to v1.7.0 (mdn#1108)
  Estelle/morestrings (mdn#1095)
  Added method for String[@@iterator]() (mdn#1069)
  Replace header image with a CC0 one (mdn#1100)
  chore(deps): update dependency prettier to v1.14.2 (mdn#1107)
  chore(deps): update dependency clean-css to v4.2.1 (mdn#1096)
  Update main element example to use cc0 image (mdn#1092)
  Update cc0 figcaption (mdn#1091)
  Fix meta.json for contenteditabl (mdn#1106)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants