Skip to content

Feature/treat warning messages as warnings#284

Merged
schoinh merged 31 commits intomainfrom
local-viewer-install
Nov 13, 2021
Merged

Feature/treat warning messages as warnings#284
schoinh merged 31 commits intomainfrom
local-viewer-install

Conversation

@schoinh
Copy link
Copy Markdown
Contributor

@schoinh schoinh commented Nov 11, 2021

Problem

Now that we get warning, info, and error messages differentiated from the viewer, we should treat warnings and info messages differently from error messages.

Closes #269

Solution

  1. Currently, the setStatus action does 2 things: set the viewer status (empty, loading, error, or success) AND set the error message in state. Setting the viewer status to "error" has effects we don't want with a warning or info (for example clearing out the agent panel), and in fact we don't want to change the viewer status when we get a warning or info at all. So I split up setStatus into setStatus and setError. Now, setStatus only sets the viewer status (empty, loading, error, or success) and setError puts the error message (with various error levels like warning, info, error) in our state so they can be turned into notifications for the user.
  2. Modify the ErrorNotification component so that it displays warning and info icons (instead of a red X) for warnings and infos

Other stuff

  • Installed esbuild-jest and esbuild (required by esbuild-jest) to modify the jest config, which fixed this annoying problem we've had forever, where the test suite would break if we imported @aics/simularium-viewer into src/state/trajectory/logics.ts. This was necessary in this PR because I needed to import simularium-viewer into that logics file! (I'm honestly not sure why this config fixed the problem or why the problem existed in the first place, it was just a lot of trial and error)

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)
  • This change requires updated or new tests

Steps to Verify:

  1. Pull this branch and npm i then npm start
  2. Load a file that needs local geometry files, but don't supply the geometry files
  3. You should see some warning messages pop up (instead of error). Everything else should work as before.

Screenshots (optional):

image

Keyfiles (delete if not relevant):

  1. src/state/viewer/reducer.ts & src/state/viewer/types.ts - separate out setStatus into setStatus and setError
  2. src/components/ErrorNotification/index.tsx - take error level into account when rendering a notification

Almost everything else is just having to update code everywhere setStatus has been used.

meganrm and others added 27 commits December 17, 2020 13:04
@schoinh schoinh requested a review from blairlyons as a code owner November 11, 2021 03:16
@schoinh schoinh requested a review from a user November 11, 2021 03:16
@schoinh schoinh requested a review from meganrm as a code owner November 11, 2021 03:16
@schoinh schoinh requested a review from toloudis as a code owner November 11, 2021 03:16
Comment thread package.json
"react-waypoint": "^9.0.3",
"redux": "4.0.1",
"redux-logic": "^2.1.1",
"redux-logic": "^3.0.3",
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I updated this to try to fix something I think is a bug with redux -- it didn't fix it, but it didn't break anything else either, so I kept the update

<NetworkFileFailedText />
) : (
checkboxTree
<NoTypeMappingText />
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is rolling back the temporary fix I put in for #274

@github-actions
Copy link
Copy Markdown

github-actions bot commented Nov 11, 2021

jest coverage report 🧪

Total coverage

Status Category Percentage Covered / Total
🟡 Statements 74.7% (-1.3% 🔻) 514/688
🟡 Branches 70.94% 105/148
🔴 Functions 43.1% (-1.81% 🔻) 75/174
🟡 Lines 74.66% (-1.34% 🔻) 495/663

Status of coverage: 🟢 - ok, 🟡 - slightly more than threshold, 🔴 - under the threshold

Show files with reduced coverage 🔻

Reduced coverage

Status Filename Statements Branches Functions Lines
🔴 actions.ts 53.84% 100% 0% 53.84%
🔴 reducer.ts 26.31% (-7.02% 🔻) 0% 0% 26.31% (-7.02% 🔻)
🔴 src/state/trajectory 37.42% (-1.08% 🔻) 0% 4.44% 37.42% (-1.08% 🔻)
🔴 src/state/viewer 58.73% (-3.77% 🔻) 0% 5% 58.73% (-3.77% 🔻)

Status of coverage: 🟢 - ok, 🟡 - slightly more than threshold, 🔴 - under the threshold

Comment thread src/state/viewer/types.ts
Comment thread src/state/viewer/reducer.ts Outdated
Comment thread src/state/viewer/reducer.ts Outdated
return {
...state,
status: action.payload.status,
error: "", // Clear out error
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

set this to initial state

Copy link
Copy Markdown
Contributor Author

@schoinh schoinh Nov 11, 2021

Choose a reason for hiding this comment

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

You mean (pseudocode) return {...initialState}? I don't think we want to do that because we want to set the status to whatever the payload.status is. (like loading, empty, success -- the ones that aren't error)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

no I meant, error: initialState.error

dispatch({
payload: { newFile: true },
type: CLEAR_SIMULARIUM_FILE,
batch(() => {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👍🏻

Copy link
Copy Markdown
Contributor

@meganrm meganrm left a comment

Choose a reason for hiding this comment

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

I just tested this with a file I have that currently crashes staging, and it works, so thumbs up. I did leave one comment that I think it's best to always be setting things in state to either something coming through the payload or to initialState[key] just so it's clear what's happening, but otherwise, looks good

@schoinh schoinh merged commit 4aaf17e into main Nov 13, 2021
@schoinh schoinh deleted the local-viewer-install branch November 13, 2021 00:00
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.

Handle different types of messages (error, warning, info) from viewer

3 participants