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

Report more errors to the user #8623

Merged

Conversation

MichaelMauderer
Copy link
Contributor

@MichaelMauderer MichaelMauderer commented Dec 22, 2023

Pull Request Description

Closes #8524 .

Screencast.from.21-12-23.14.52.20.webm

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

  • The documentation has been updated, if necessary.
  • Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
  • All code follows the
    Scala,
    Java,
    and
    Rust
    style guides. In case you are using a language not listed above, follow the Rust style guide.
  • All code has been tested:
    • Unit tests have been written where possible.
    • If GUI codebase was changed, the GUI was tested when built using ./run ide build.

@MichaelMauderer MichaelMauderer added CI: No changelog needed Do not require a changelog entry for this PR. -gui labels Dec 22, 2023
@MichaelMauderer MichaelMauderer self-assigned this Dec 22, 2023
@MichaelMauderer MichaelMauderer force-pushed the wip/MichaelMauderer/gui2/Report-errors-to-the-user branch 2 times, most recently from 4b31b03 to 8d9c39d Compare December 22, 2023 12:31
@MichaelMauderer MichaelMauderer force-pushed the wip/MichaelMauderer/gui2/Report-errors-to-the-user branch 2 times, most recently from e281c74 to 2fe3ffc Compare December 22, 2023 13:34
@MichaelMauderer MichaelMauderer force-pushed the wip/MichaelMauderer/gui2/Report-errors-to-the-user branch from 2fe3ffc to 15e206b Compare December 22, 2023 13:42
@MichaelMauderer MichaelMauderer marked this pull request as ready for review December 22, 2023 13:43
Copy link
Contributor

@somebody1234 somebody1234 left a comment

Choose a reason for hiding this comment

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

approving the (minimal) dashboard changes. also minor nits as usual

})
}

function initiConnectionLostToast() {
Copy link
Contributor

Choose a reason for hiding this comment

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

typo?

@@ -62,6 +62,7 @@
"semver": "^7.5.4",
"sucrase": "^3.34.0",
"vue": "^3.3.4",
"vue-react-wrapper": "^0.3.1",
Copy link
Contributor

Choose a reason for hiding this comment

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

i think this should be a devDependency (npm i -D vue-react-wrapper) right? since it's only used during development (histoire)

app/gui2/src/components/GraphEditor.vue Show resolved Hide resolved
toastId: connectionLostToast,
})
},
{ once: true },
Copy link
Contributor

Choose a reason for hiding this comment

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

how did i not know about this option o_o

})
}

projectStore.lsRpcConnection.catch((err) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

you can provide the catch callback as the second argument to the .then() below.

@@ -32,6 +35,20 @@ main =
result = run_main benches
third_node = 2 + 2
`)

/// Note: These props should be synced with the props in
Copy link
Contributor

Choose a reason for hiding this comment

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

jsdocs are formatted like this:

/**
 * docs here
 */

otherwise just a double slash on the first line is fine


/// Note: These props should be synced with the props in
// `app/ide-desktop/lib/dashboard/src/authentication/src/components/app.tsx`.
// We need this here, as the rect component is not part of the dashboard and not usually available in the demo scenes.
Copy link
Contributor

Choose a reason for hiding this comment

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

is rect a typo?

toastClassName: 'text-sm leading-170 bg-frame-selected rounded-2xl backdrop-blur-3xl',
limit: 3,
})
const JSONView = createReactWrapper(ToastContainer, toastProps)
Copy link
Contributor

Choose a reason for hiding this comment

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

just want to confirm that name of JSONView is intentional?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is at all necessary. The react app already renders toast container, so issued toasts will already be linked to it automatically. We don't need to render additional container.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed the name to something more sensible.

@Frizi This is our test environment. There is otherwise no react app running there, as we don't initialise the dashboard here, and thus no toast appears. Adding this, ensures the toast container is initialised in the demo scene and allows them to appear. Unless I am missing something?

@MichaelMauderer MichaelMauderer merged commit cd30815 into develop Jan 4, 2024
29 of 30 checks passed
@MichaelMauderer MichaelMauderer deleted the wip/MichaelMauderer/gui2/Report-errors-to-the-user branch January 4, 2024 13:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-gui CI: No changelog needed Do not require a changelog entry for this PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Report errors to the user
3 participants