Skip to content

[Auth] Update documentation to fill gaps #4743

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Apr 7, 2021
Merged

[Auth] Update documentation to fill gaps #4743

merged 4 commits into from
Apr 7, 2021

Conversation

sam-gc
Copy link
Contributor

@sam-gc sam-gc commented Apr 7, 2021

No description provided.

@sam-gc sam-gc requested review from Feiyang1 and egilmorez April 7, 2021 01:52
egilmorez
egilmorez reviewed Apr 7, 2021
View reviewed changes
Copy link
Contributor

@egilmorez egilmorez left a comment

Choose a reason for hiding this comment

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

Some nits for you Sam, thanks!

packages-exp/auth-exp/src/core/auth/initialize.ts Outdated
* @remarks
*
* This function allows more control over the Auth instance than
* {@link getAuth}. `getAuth` will use platform-specific defaults to supply
Copy link
Contributor

Choose a reason for hiding this comment

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

Global nit: we try to avoid "will" unless we really mean the future.

I think this, and in line 36, and probably elsewhere, are fine as present tense: uses, works, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

packages-exp/auth-exp/src/core/auth/initialize.ts Outdated
* size if you're not using either `signInWithPopup` or `signInWithRedirect`.
*
* For example, if your app only uses anonymous accounts and you only want
* accounts saved for the current session, you would initialize Auth thus:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest just commanding and replacing "thus" with words describing the difference (thus is a little over formal for Google).

Like, "If your app uses only anonymous . . . , initialize it with :"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

packages-exp/auth-exp/src/core/credentials/phone.ts Outdated
@@ -121,7 +121,7 @@ export class PhoneAuthCredential extends AuthCredential {
return obj;
}

/** {@inheritdoc AuthCredential.fromJSON} */
/** Generates a Phone credential based on a plain object or a JSON string. */
Copy link
Contributor

Choose a reason for hiding this comment

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

Literal? Backtick if so.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No literal here

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we lowercase "phone" in that case?

packages-exp/auth-exp/src/core/providers/saml.ts Outdated
@@ -46,6 +46,22 @@ export class SAMLAuthProvider extends FederatedAuthProvider {
super(providerId);
}

/**
* Generates an {@link AuthCredential} from a {@link UserCredential} after a
* successful SAML flow ompletes.
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

packages-exp/auth-exp/src/model/public_types.ts Outdated
*
* There are two ways to initialize an auth instance: {@link getAuth} and
* {@link initializeAuth}. `getAuth` initializes everything using
* platform-specific configurations, while `initializeAuth` takes a Dependencies
Copy link
Contributor

Choose a reason for hiding this comment

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

Literal?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

packages-exp/auth-exp/src/model/public_types.ts Outdated
* @public
*/
export interface Dependencies {
/**
* Which {@link Persistence} to use. If this is an array, the first
* Persistence that the device supports is used. The SDK will search for an
Copy link
Contributor

Choose a reason for hiding this comment

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

Literal Persistence here and below?

Lots of "will" here too :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done and done

@changeset-bot
Copy link

changeset-bot bot commented Apr 7, 2021 •
edited
Loading

⚠️ No Changeset found

Latest commit: e47c732

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

egilmorez
egilmorez approved these changes Apr 7, 2021
View reviewed changes
Copy link
Contributor

@egilmorez egilmorez left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

packages-exp/auth-exp/src/core/credentials/phone.ts Outdated
@@ -121,7 +121,7 @@ export class PhoneAuthCredential extends AuthCredential {
return obj;
}

/** {@inheritdoc AuthCredential.fromJSON} */
/** Generates a Phone credential based on a plain object or a JSON string. */
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we lowercase "phone" in that case?

@sam-gc sam-gc assigned Feiyang1 and unassigned egilmorez Apr 7, 2021
@google-oss-bot
Copy link
Contributor

Size Analysis Report

Affected Products

  • @firebase/auth-exp

    • EmailAuthCredential

      Size Table

      TypeBase (f24d896)Head (95c1dc0)Diff
      size
      31.6 kB
      31.6 kB
      +14 B (+0.0%)
      size-with-ext-deps
      42.5 kB
      42.5 kB
      +14 B (+0.0%)

    • EmailAuthProvider

      Size Table

      TypeBase (f24d896)Head (95c1dc0)Diff
      size
      33.0 kB
      33.0 kB
      +14 B (+0.0%)
      size-with-ext-deps
      44.2 kB
      44.2 kB
      +14 B (+0.0%)

    • signInWithEmailAndPassword

      Size Table

      TypeBase (f24d896)Head (95c1dc0)Diff
      size
      34.5 kB
      34.5 kB
      +14 B (+0.0%)
      size-with-ext-deps
      45.6 kB
      45.7 kB
      +14 B (+0.0%)

    • signInWithEmailLink

      Size Table

      TypeBase (f24d896)Head (95c1dc0)Diff
      size
      34.7 kB
      34.7 kB
      +15 B (+0.0%)
      size-with-ext-deps
      45.8 kB
      45.9 kB
      +15 B (+0.0%)

@Feiyang1 Feiyang1 merged commit bd7277d into master Apr 7, 2021
@Feiyang1 Feiyang1 deleted the samgho/docs branch April 7, 2021 19:03
@firebase firebase locked and limited conversation to collaborators May 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Feiyang1 Feiyang1 Feiyang1 approved these changes

@egilmorez egilmorez egilmorez approved these changes

@yuchenshi yuchenshi yuchenshi approved these changes

@avolkovi avolkovi Awaiting requested review from avolkovi

@hiranya911 hiranya911 Awaiting requested review from hiranya911

@hsubox76 hsubox76 Awaiting requested review from hsubox76

Assignees

@Feiyang1 Feiyang1

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@sam-gc @google-oss-bot @Feiyang1 @egilmorez @yuchenshi