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.

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

Clean up measurements Redux states/actions #1881

Merged
merged 12 commits into from
Nov 6, 2024
+181 −159
Merged

Clean up measurements Redux states/actions #1881

Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
d9e679d
Remove excess whitespace
joverlee521 Oct 25, 2024
7599aa7
Fetch and load measurements sidecar JSON upfront
joverlee521 Oct 28, 2024
047de26
Remove unused `LOAD_MEASUREMENTS` action
joverlee521 Oct 28, 2024
a5bc80f
Remove unused `UPDATE_MEASUREMENTS_ERROR` action
joverlee521 Oct 29, 2024
0f67361
Remove `controls.measurementsCollectionKey` Redux state
joverlee521 Oct 29, 2024
0861269
parseMeasurementsJSON: deep clone `json.collections`
joverlee521 Oct 28, 2024
a2431ae
measurements: display error even if data not loaded
joverlee521 Oct 28, 2024
947eda6
Add `combineMeasurementsControlsAndQuery`
joverlee521 Oct 28, 2024
6c2308a
Hide measurements panel if loading failed
joverlee521 Oct 30, 2024
cab837e
Measurements: Switch conditionals for rendering
joverlee521 Nov 5, 2024
0069e09
loadMeasurements: fix bug for warning notifications
joverlee521 Nov 5, 2024
c03d336
Remove measurements query params if loading failed
joverlee521 Nov 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 13 additions & 14 deletions src/components/measurements/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -343,20 +343,19 @@ const Measurements = ({height, width, showLegend}) => {

return (
<Card infocard={showOnlyPanels} title={title} titleStyles={getCardTitleStyle()}>
{measurementsLoaded &&
(measurementsError ?
<Flex style={{ height, width}} direction="column" justifyContent="center">
<p style={{ textAlign: "center" }}>
{measurementsError}
</p>
</Flex> :
<MeasurementsPlot
height={height}
width={width}
showLegend={showLegend}
setPanelTitle={setTitle}
/>
)
{measurementsError ?
<Flex style={{ height, width}} direction="column" justifyContent="center">
<p style={{ textAlign: "center" }}>
{measurementsError}
</p>
</Flex> :
(measurementsLoaded &&
<MeasurementsPlot
height={height}
width={width}
showLegend={showLegend}
setPanelTitle={setTitle}
/>)
Copy link
Member

Choose a reason for hiding this comment

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

[for consideration]

The way this is written invites the question "what happens if there's no error but it's not loaded?". With the move to synchronous loading the state generated by loadMeasurements is either (loaded: true, error: undefined) or (loaded: false, error: warningMessage), but it's plausible the code will change later on and throw these two values out-of-sync. I'd suggest flipping the conditionals here: if loaded render the panel, if not show an error component with measurementsError || "unknown error".

Update: 6c2308a removes the entire panel if loaded: false, so these conditionals are never in play. We'll still get a warning notification "Failed to parse measurements collections" but no panel.

I'll leave the decision to you whether we display an error panel or just the notification and let you update the code accordingly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd suggest flipping the conditionals here: if loaded render the panel, if not show an error component with measurementsError || "unknown error".

I'll go with this!

Update: 6c2308a removes the entire panel if loaded: false, so these conditionals are never in play. We'll still get a warning notification "Failed to parse measurements collections" but no panel.

It loads the page without the panel, but the user can still toggle on the panel via the sidebar controls. I think it's helpful to display the error message if the user decides to toggle on the panel after the warning notification has disappeared.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in cab837e

}
</Card>
);
Expand Down