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

Correct "expected output" mistake #1000

Merged
merged 2 commits into from
Jun 28, 2018
Merged

Conversation

Arkangus
Copy link
Contributor

No description provided.

@welcome
Copy link

welcome bot commented Jun 27, 2018

💖 Thanks for opening this pull request! 💖
Here is a list of things that will help get it across the finish line: - If this is a new or updated CSS interactive example, please ensure that you followed the CSS styleguide - If this is a new or updated JavaScript interactive example, please ensure that you followed the JavaScript styleguide - If your changes affects any of the steps in our contribution docs, please also make the relevant changes there.

Copy link

@schalkneethling schalkneethling left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution @Arkangus

It seems you might have identified a larger problem with this example. I have made a suggestion on a change.

@@ -3,7 +3,7 @@
var reversed = array1.reverse();

console.log(array1);
// expected output: Array ['three', 'two', 'one']
// expected output: Array ['one', 'two', 'three']

console.log(reversed);
// expected output: Array ['three', 'two', 'one']

Choose a reason for hiding this comment

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

@Arkangus This is actually a little trickier and perhaps we should rewrite the example a bit. The reverse function is actually destructive i.e. it changes in place so, effects the original array as well. So, the output that it shown, is what is printed but, might be unexpected.

I therefore propose we change the example to the following:

var array1 = ['one', 'two', 'three'];
console.log('array1: ', array1);
// expected output: Array ['one', 'two', 'three']

var reversed = array1.reverse(); 
console.log('reversed: ', reversed);
// expected output: Array ['three', 'two', 'one']

/* Because `reverse`'s changes are done in place it
   also changes the original array, as can be seen here */
console.log('array1: ', array1);
// expected output: Array ['three', 'two', 'one']

Thoughts @wbamberg ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for pointing that out. I definitely should have checked the actual answer!

I agree with the change. I would just replace

/* Because 'reverse''s changes are done in place it also changes the original array, as can be seen here */

By

/* Careful: reverse is destructive. It changes the original array */

I think it's clearer.

Copy link

@schalkneethling schalkneethling left a comment

Choose a reason for hiding this comment

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

r+ Thanks @Arkangus

@schalkneethling schalkneethling merged commit 19639b4 into mdn:master Jun 28, 2018
wbamberg pushed a commit to wbamberg/interactive-examples that referenced this pull request Jul 12, 2018
* upstream/master: (33 commits)
  Add HTML example for <a> (issue mdn#1014) (mdn#1022)
  fix(tabbed): load fonts as part of the editor css as @font-face does not work inside shadow dom (mdn#1015)
  html/input: Lowercase attributes (mdn#1024)
  Add color-adjust property example (mdn#1008)
  chore(deps): update dependency eslint to v5.1.0 (mdn#1023)
  chore(community): add @goodwin64 as contributor (mdn#1021)
  chore(community): add @dagolinuxoid as contributor (mdn#1020)
  chore(community): add @arai-a as contributor (mdn#1019)
  chore(community): add @ro-ka as contributor (mdn#1018)
  Issue mdn#1013 <style> elements in HTML editor break editor's render method (mdn#1017)
  chore(deps): update jest monorepo to v23.3.0 (mdn#1012)
  Issue#946 open links in new tab (mdn#1009)
  Send metric only during loading event (mdn#1011)
  fix(performance): only send loading mark inside loading event (mdn#1010)
  chore(deps): update dependency prettier to v1.13.7 (mdn#1006)
  fix(address): add name to the provided address (mdn#1004)
  chore(community): add @Arkangus as contributor (mdn#1002)
  Correct "expected output" mistake (mdn#1000)
  fix(performance): send post to kuma for custom ie-load-event-end metric (mdn#1001)
  chore(deps): update jest monorepo to v23.2.0 (mdn#993)
  ...
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