Fix merge conflict and some additional typos - #43
Conversation
There was a problem hiding this comment.
There are many small changes that are not really useful to fix. For example regular comments, and comments in tests. The problem with them is that it’s time consuming to check them, and the grammar fix doesn’t really alp at all.
I’ll approve the, this time, but please avoid investing time in such small changes.
All other changes are good, and welcome.
I’ll wait for any additional feedback from the community as English is not my first language, and please fix the requested changes.
Thank you!
| 1. It is more secure. The reason you need to whitelist in the first place is for security. | ||
| 2. It is way faster, in some cases up to 5 seconds faster. | ||
| 3. I don't trust firebase (or anyone) with my user's private data, and you shouldn't either. | ||
| 1. More secure, it's the reason you need to whitelist in the first place is for security. |
There was a problem hiding this comment.
This fix is not good. The sentence doesn’t make sense. It should be:
It’s more secure. That’s the reason you need to whitelist in the first place is for security.
| 2. It is way faster, in some cases up to 5 seconds faster. | ||
| 3. I don't trust firebase (or anyone) with my user's private data, and you shouldn't either. | ||
| 1. More secure, it's the reason you need to whitelist in the first place is for security. | ||
| 2. Way faster, in some cases up to 5 seconds faster. |
There was a problem hiding this comment.
Here too. Omitting “It’s” doesn’t make sense.
| window.addEventListener('storage', e => { | ||
| // This code will run if localStorage for this user | ||
| // data was updated from a different browser window. | ||
| // This code will run if the local storage for this user |
There was a problem hiding this comment.
The meaning was literally “localStorage” as in the API, so this one needs to remain as it was.
|
|
||
| /** | ||
| * Makes a post request to a specific endpoint and returns the response. | ||
| * Makes post request to a specific endpoint and return the response. |
There was a problem hiding this comment.
Here too it should be as it was.
If you are checking this with grammarly or something similar it will think that “post” is a verb, but it isn’t, so it doesn’t make sense to remove “a”. And it should also remain “returns”.
|
|
||
| // If the response returned an error, try to get a Firebase error code/message. | ||
| // Sometimes the error codes are joined with an explanation, we don't need that(its a bug). | ||
| // Sometimes the error codes are joined with an explanation, we don't need that. |
There was a problem hiding this comment.
The comment that’s it’s a bug is important so that future contributors know why the code does what it does. If it gets fixed in the future it will affect the way we want to handle it
| // (No docs on this...) | ||
| await this.storage.set(this.sKey('SessionId'), sessionId); | ||
| // Save if this is a fresh log-in or a "link account" request. | ||
| // Save if this is a fresh signed in or a "link account" request. |
There was a problem hiding this comment.
Should be “sign in” not “signed in”
| // Check for the edge case in which the user signed out | ||
| // before completing the linkAccount request. | ||
| if (linkAccount && !this.user) throw Error('Request to "Link account" was made, but user is no longer signed-in'); | ||
| if (linkAccount && !this.user) throw Error('Request to "Link account" was made, but user is no longer signed in'); |
There was a problem hiding this comment.
Changing error messages should be done separately or with a notice as some users might depend on the exact string.
Since this is a beta I’ll approve it but it’s can usually be considered a breaking change.
There was a problem hiding this comment.
This is important to know, I didn't know. Thanks.
|
|
||
| /** | ||
| * Sends an out-of-band confirmation code for an account. | ||
| * It can be used to reset a password, to verify an email address and send a sign-in email link. |
There was a problem hiding this comment.
Sign-in is used as a noun here, so the hyphen should remain.
| * @param {Object} newData An object with the new data. | ||
| * @throws Will throw if the user is not signed-in. | ||
| * @throws Will throw if the user is not signed in. | ||
| * Update user's profile. |
There was a problem hiding this comment.
Again. The description should be on top. Please revert
|
Thank you for your patient, I've fixed the requested changes. Anyway, feel free to cherry pick changes, if at all. |
awinograd
left a comment
awinograd
left a comment
There was a problem hiding this comment.
Most of the changes are stylistic / personal preference. There are a handful of grammar fixes. Nothing harmful in my opinion
| } | ||
|
|
||
| /** | ||
| * Emits an event and triggers all the listeners. |
There was a problem hiding this comment.
I removed the other one (line78), thanks.
| * Updates the user data in localStorage. | ||
| * @param {Object} userData New user data. | ||
| * @param {boolean} [updateStorage = true] Check whether to update localStorage or not. | ||
| * @param {boolean} [persist = true] Whether to update local storage or not. |
There was a problem hiding this comment.
should be localStorage for consistency with the rest of comments
There was a problem hiding this comment.
true, I'll fix it and another one in line 260.
3 participants
@samuelgozi I've fixed the merge conflicts and I added some additional typo fixes.