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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft: What? #285

Open
wants to merge 8 commits into
base: release
Choose a base branch
from
Open

Draft: What? #285

wants to merge 8 commits into from

Conversation

draymond63
Copy link
Contributor

Implementation Description

Brief summary of PR

Screenshots

Visual depiction of PR

What should reviewers focus on?

Testing Procedure

  • What test(s) should we run to make sure your code works?
    • What ways could the input be wrong?
    • Is it secure? Could nefarious users access/break it?

Things to Note

  • Additional dependencies/libraries you've added?
  • Things you'd want to know when coming back to the code after a few months

Checklist

  • Ensure code follows style guide by running the linters
  • I have updated the documentation or deemed it unnecessary
  • I have linked the relevant issue in this PR
  • I have requested a review from the PL, as well as other dev(s) (and designers if necessary)

dependabot bot and others added 8 commits December 17, 2022 12:51
Bumps [express](https://github.com/expressjs/express) from 4.17.1 to 4.17.3.
- [Release notes](https://github.com/expressjs/express/releases)
- [Changelog](https://github.com/expressjs/express/blob/master/History.md)
- [Commits](expressjs/express@4.17.1...4.17.3)

---
updated-dependencies:
- dependency-name: express
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <[email protected]>

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [mongoose](https://github.com/Automattic/mongoose) from 5.12.12 to 5.13.15.
- [Release notes](https://github.com/Automattic/mongoose/releases)
- [Changelog](https://github.com/Automattic/mongoose/blob/master/CHANGELOG.md)
- [Commits](Automattic/mongoose@5.12.12...5.13.15)

---
updated-dependencies:
- dependency-name: mongoose
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
@draymond63 draymond63 requested review from a team and jfdoming as code owners May 31, 2023 04:12
@draymond63 draymond63 requested a review from y2kwak May 31, 2023 04:12
Copy link
Contributor

@jfdoming jfdoming left a comment

Choose a reason for hiding this comment

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

All looks good! Just some quick thoughts


if (mutationError) {
saveToast({
title: "An error occured while saving.",
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add "Please try again later!" to be friendlier?

@@ -77,6 +77,7 @@ const refreshDirectionalLink = new RetryLink().split(
"Refresh",
"ResetPassword",
"Login",
"Logout",
Copy link
Contributor

Choose a reason for hiding this comment

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

Surprised this didn't cause problems before?

// if (networkError?.toString() === "Error: Failed to refresh access token!") {
// window.location.href = "/logout";
// }
if (networkError) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems a little risky since I'd imagine this would also sign people out if their internet briefly disconnected. Is there a particular reason to change the logic here? (i.e., was it not working before?)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants