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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added more helpful technique for accessing promise #1124

Merged
merged 3 commits into from Sep 13, 2018

Conversation

Projects
None yet
2 participants
@justsml
Copy link
Contributor

commented Sep 3, 2018

Showing that a promise evaluates to the string descriptor: [object Promise] is not the most helpful thing to include in the first example.

Let me know if I need to revise, thanks for any feedback! 馃挴

@welcome

This comment has been minimized.

Copy link

commented Sep 3, 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.
@@ -1,9 +1,23 @@
<pre>
<code id="static-js">var promise1 = new Promise(function(resolve, reject) {
setTimeout(resolve, 100, 'foo');

This comment has been minimized.

Copy link
@justsml

justsml Sep 3, 2018

Author Contributor

Using setTimeout like this, with a 3rd argument ('foo') - is IMHO rather rare.


// How to show the value of promise1:

promise1.then(value => console.log(value));

This comment has been minimized.

Copy link
@justsml

justsml Sep 3, 2018

Author Contributor

I know this is the constructor example- I feel this bit is needed because people see constructor in their search, then think assume they will find how to make (and perhaps read) promises there." Well... Not exactly on this page. 馃樋

While there are good supporting links and contextual info, it can easily be missed, especially the first time you land on the example/page.

The .then() line is a far more common use case, and I hope will benefit many users. 馃

Note: I wouldn't add anything extraneous like .catch()

This comment has been minimized.

Copy link
@wbamberg

wbamberg Sep 7, 2018

Member

Yeah, I can see the value of showing how to use the promise in this example.

(really I suspect about 95% of the people who land on this page should be reading https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Using_promises instead)

@wbamberg
Copy link
Member

left a comment

I like the idea of showing how to use a promise in this example. Thanks!

But I don't think it needs the explanatory comments. I also had a couple of comments on conventions and consistency with arrow functions.

@@ -1,9 +1,23 @@
<pre>
<code id="static-js">var promise1 = new Promise(function(resolve, reject) {
setTimeout(resolve, 100, 'foo');
setTimeout(function() {

This comment has been minimized.

Copy link
@wbamberg

wbamberg Sep 7, 2018

Member

I think it's weird to mix arrow functions and non-arrow functions, is there a reason you're doing this?

This comment has been minimized.

Copy link
@justsml

justsml Sep 11, 2018

Author Contributor

Let me know if I need to do any other changes. Thanks for your time.

setTimeout(resolve, 100, 'foo');
setTimeout(function() {
resolve('foo');
}, 100);

This comment has been minimized.

Copy link
@wbamberg

wbamberg Sep 7, 2018

Member

If we want to do this we should bump this a bit so the delay is noticeable (but not annoying). 300 or 400 ms would work better.

});


// How to show the value of promise1:

This comment has been minimized.

Copy link
@wbamberg

wbamberg Sep 7, 2018

Member

This comment isn't needed.


// How to show the value of promise1:

promise1.then(value => console.log(value));

This comment has been minimized.

Copy link
@wbamberg

wbamberg Sep 7, 2018

Member

Yeah, I can see the value of showing how to use the promise in this example.

(really I suspect about 95% of the people who land on this page should be reading https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Using_promises instead)

// How to show the value of promise1:

promise1.then(value => console.log(value));
//=> "foo"

This comment has been minimized.

Copy link
@wbamberg

wbamberg Sep 7, 2018

Member

We use the convention:

// expected output: "foo"
// promise1.then(function(value) {/* handle value in function here */})

// The following example **won't work**:
// Reading Synchronously:

This comment has been minimized.

Copy link
@wbamberg

wbamberg Sep 7, 2018

Member

I don't think these comments are needed. This is an example, it is not the documentation. We should be very sparing with comments in these examples IMO - we should aim to show not tell. There are plenty of words in the rest of the page for people to read.

console.log(promise1);
// expected output: [object Promise]
//=> [object Promise]

This comment has been minimized.

Copy link
@wbamberg

wbamberg Sep 7, 2018

Member

Again, we use the convention:

// expected output: [object Promise]
@justsml

This comment has been minimized.

Copy link
Contributor Author

commented Sep 9, 2018

Thanks for the feedback @wbamberg !

@wbamberg
Copy link
Member

left a comment

Thanks @justsml . There was just a missing semicolon, which I fixed myself :) . Otherwise this looks great.

@wbamberg wbamberg merged commit 541dafc into mdn:master Sep 13, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
security/snyk - package.json (schalkneethling) No new issues
Details
@welcome

This comment has been minimized.

Copy link

commented Sep 13, 2018

Congrats on merging your first pull request! 馃帀馃帀馃帀

@justsml

This comment has been minimized.

Copy link
Contributor Author

commented Sep 17, 2018

Thanks @wbamberg !!!

wbamberg added a commit to wbamberg/interactive-examples that referenced this pull request Oct 16, 2018

Merge remote-tracking branch 'upstream/master'
* upstream/master:
  Rename file as "*" is not a valid character in Windows file name (mdn#1151)
  Updating picture example to work better on small screens (mdn#1149)
  Fix incorrect label selector (mdn#1147)
  Each radio button in the group should have distinct `value` attribute (mdn#1141)
  Add example for Object.fromEntries (mdn#1127)
  Added more helpful technique for accessing promise (mdn#1124)
  Editor config fix (mdn#1143)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.