Fix merge conflict and some additional typos #43

DaniyelMe · 2020-07-07T19:57:39Z

@samuelgozi I've fixed the merge conflicts and I added some additional typo fixes.

samuelgozi

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!

samuelgozi · 2020-07-07T20:18:53Z

README.md

-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.


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.

resolved in #86991a5

samuelgozi · 2020-07-07T20:19:52Z

README.md

-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.


Here too. Omitting “It’s” doesn’t make sense.

samuelgozi · 2020-07-07T20:21:50Z

src/main.js

@@ -66,14 +66,15 @@ export default class Auth {
 		// we need to check if this environment supports `addEventListener` on the window.
 		'addEventListener' in window &&
 			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


The meaning was literally “localStorage” as in the API, so this one needs to remain as it was.

samuelgozi · 2020-07-07T20:25:43Z

src/main.js

@@ -104,7 +105,7 @@ export default class Auth {
 	}

 	/**
-	 * Makes a post request to a specific endpoint and returns the response.
+	 * Makes post request to a specific endpoint and return the response.


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”.

samuelgozi · 2020-07-07T20:27:34Z

src/main.js

@@ -122,7 +123,7 @@ export default class Auth {
 			let data = await response.json();

 			// 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.


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

samuelgozi · 2020-07-07T20:29:15Z

src/main.js

@@ -260,7 +261,7 @@ export default class Auth {
 		// Is required to finish the auth flow, I believe this is used to mitigate CSRF attacks.
 		// (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.


Should be “sign in” not “signed in”

samuelgozi · 2020-07-07T20:32:50Z

src/main.js

 		// 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');


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.

This is important to know, I didn't know. Thanks.

samuelgozi · 2020-07-07T20:36:05Z

src/main.js

@@ -361,8 +361,8 @@ export default class Auth {

 	/**
 	 * 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.


Sign-in is used as a noun here, so the hyphen should remain.

src/main.js

samuelgozi · 2020-07-07T20:37:50Z

src/main.js

 	 * @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.


Again. The description should be on top. Please revert

DaniyelMe · 2020-07-07T21:35:24Z

Thank you for your patient, I've fixed the requested changes.
It's wise to take the time and evaluate the changes by others, that's way I'm always asking @awinograd feedback.

Anyway, feel free to cherry pick changes, if at all.
Stay safe.

awinograd

Most of the changes are stylistic / personal preference. There are a handful of grammar fixes. Nothing harmful in my opinion

awinograd · 2020-07-07T22:12:39Z

src/main.js

 				if (e.key !== this.sKey('User')) return;
 				this.setState(JSON.parse(e.newValue), false);
 			});
 	}

 	/**
+	 * Emits an event and triggers all the listeners.


duplicate line

I removed the other one (line78), thanks.

awinograd · 2020-07-07T22:13:13Z

src/main.js

 		return this.refreshIdToken(); // Won't do anything if the token is valid.
 	}

 	/**
 	 * 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.


should be localStorage for consistency with the rest of comments

true, I'll fix it and another one in line 260.

DaniyelMe added 6 commits June 24, 2020 11:59

Fix some typos, missing spaces.

5e4fa34

Fix inconsistencies in the comments and some spaces.

4b488c1

reverts some mistakes. Fix some inconsistencies.

2e69674

Update to remote current state

ea454bf

Fix typos

61723f7

fix: correcting some marge conflicts leftovers

2ea2390

DaniyelMe mentioned this pull request Jul 7, 2020

Fix inconsistencies in the comments and some spaces #42

Open

samuelgozi requested changes Jul 7, 2020

View reviewed changes

fix: undo mistakes

0ee757a

awinograd approved these changes Jul 7, 2020

View reviewed changes

DaniyelMe added 3 commits July 8, 2020 10:15

remove duplication and fix inconsistencies

b37b38f

Merge 'samuelgozi/master' into fix-comments

45b6f3d

fix: resolve a change request

86991a5

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix merge conflict and some additional typos #43

Fix merge conflict and some additional typos #43

DaniyelMe commented Jul 7, 2020

samuelgozi left a comment •

edited

Loading

samuelgozi Jul 7, 2020

DaniyelMe Jul 28, 2020

samuelgozi Jul 7, 2020

samuelgozi Jul 7, 2020

samuelgozi Jul 7, 2020

samuelgozi Jul 7, 2020

samuelgozi Jul 7, 2020

samuelgozi Jul 7, 2020

DaniyelMe Jul 8, 2020

samuelgozi Jul 7, 2020

samuelgozi Jul 7, 2020

DaniyelMe commented Jul 7, 2020

awinograd left a comment

awinograd Jul 7, 2020

DaniyelMe Jul 8, 2020

awinograd Jul 7, 2020

DaniyelMe Jul 8, 2020

Fix merge conflict and some additional typos #43

Are you sure you want to change the base?

Fix merge conflict and some additional typos #43

Conversation

DaniyelMe commented Jul 7, 2020

samuelgozi left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DaniyelMe commented Jul 7, 2020

awinograd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samuelgozi left a comment •

edited

Loading