-
-
Notifications
You must be signed in to change notification settings - Fork 655
Added Promises content in introduction & about #1397
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
Conversation
junedev
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for working on this. I left a couple of minor comments.
concepts/promises/about.md
Outdated
| }); | ||
| ``` | ||
|
|
||
| ## Methods |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would recommend to first show the instance methods and then the class methods and naming the headers accordingly. Also you can use h3 headings instead of bold text for the sub-sections. The structure could look like this:
## Instance Methods of a Promise
### then
### catch
### finally
## Static Methods of the Promise Class
[here there should be some text with a link to all the other methods as we don't show all of them, https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise#static_methods]
### Promise.all
### Promise.reject
### Promise.resolve| @@ -1,4 +1,104 @@ | |||
| # Introduction | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The content of this file also needs to be copied to the introduction.md file of the translation service exercise: https://github.com/exercism/javascript/blob/main/exercises/concept/translation-service/.docs/introduction.md
concepts/promises/about.md
Outdated
|
|
||
| When either of these options happens, the associated handlers queued up by a promise's `then` method is called. If the promise has already been fulfilled or rejected when a corresponding handler is attached, the handler will be called, so there is no race condition between an asynchronous operation completing and its handlers being attached. | ||
|
|
||
| The methods [`promise.then()`][promise-then], [`promise.catch()`][promise-catch], and [`promise.finally()`][promise-finally] are used to associate further action with a promise that becomes settled. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sentence can be removed as those methods are explained in detail below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I also delete line number 11?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I think it is ok to keep it as general explanation.
concepts/promises/about.md
Outdated
| A Promise is in one of these states: | ||
|
|
||
| - **pending**: initial state, neither fulfilled nor rejected. | ||
| - **fulfilled**: meaning that the operation was completed successfully. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| - **fulfilled**: meaning that the operation was completed successfully. | |
| - **fulfilled**: meaning that the operation was completed successfully and the result is available. |
concepts/promises/about.md
Outdated
| # About | ||
|
|
||
| TODO: add information on promises concept | ||
| The [Promise][promise-docs] object represents the eventual completion (or failure) of an asynchronous operation and its resulting value. It allows you to associate handlers with an asynchronous action's eventual success value or failure reason. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Links can be distracting so they should be used sparingly and only if they add additional value compared to the existing content. E.g. the link to the promise docs above could be placed in the links.json instead (if its not included already). It will then render below the content of this file in the "Read More" section.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is already a link to https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Using_promises
Should I also add another link to https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think they are different enough so that makes sense.
- Changed the ordering of methods - Fixed minor content issues
|
Anything more to add/change? |
|
Well, there are probably always minor things that could be discussed/rephrased/changed. But the main thing I look for is that the proposed change is an improvement to what was there before (which this definitely is), that it is applied consistently across the different files (like with the exercise introduction) and whether there is something major that needs to be fixed the the headlines in this case. |
Fixes #1384