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

Update metadata #3361

Merged
merged 6 commits into from
Mar 28, 2023
Merged

Conversation

MVarshini
Copy link
Contributor

@MVarshini MVarshini commented Mar 25, 2023

This PR fixes the following bugs:

Update metadata API changes
Table Border for select box column
Selecting Saved and New Runs

@MVarshini MVarshini added the Dashboard Of and relating to the Dashboard GUI label Mar 25, 2023
@MVarshini MVarshini added this to the v0.73 milestone Mar 25, 2023
@MVarshini MVarshini requested review from riya-17 and dbutenhof March 25, 2023 07:23
@MVarshini MVarshini self-assigned this Mar 25, 2023
dbutenhof
dbutenhof previously approved these changes Mar 27, 2023
Copy link
Member

@dbutenhof dbutenhof left a comment

Choose a reason for hiding this comment

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

I'm approving, because catching the dashboard up to the API changes is important (and sorry for missing that when I change the API 😢 ); but I also have some concerns/suggestions/questions ...

dashboard/src/actions/overviewActions.js Outdated Show resolved Hide resolved
dbutenhof
dbutenhof previously approved these changes Mar 27, 2023
@dbutenhof dbutenhof merged commit 8932900 into distributed-system-analysis:main Mar 28, 2023
Comment on lines +142 to +146
let errorText = "";

for (const [key, value] of Object.entries(errors)) {
errorText += `${key} : ${value} \n`;
}
Copy link
Member

Choose a reason for hiding this comment

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

Alternately,

          const errorText = Object.entries(errors).reduce(
            (a, [k, v]) => a += `${k} : ${v} \n`,
            ""
            );

Comment on lines +209 to +214
export const setSelectedSavedRuns = (rows) => {
return {
type: TYPES.SELECTED_SAVED_RUNS,
payload: rows,
};
};
Copy link
Member

Choose a reason for hiding this comment

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

Alternately,

export const setSelectedSavedRuns = (rows) => (
  {
    type: TYPES.SELECTED_SAVED_RUNS,
    payload: rows,
  }
);

That is, we only need to return a value which is an object -- we don't need any actual code (like a return statement).

@@ -1 +1,2 @@
export const DANGER = "danger";
export const ERROR_MSG = "Something went wrong!";
Copy link
Member

Choose a reason for hiding this comment

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

This is expedient, but it would be really handy if we could come up with something less generic. Any context would potentially be helpful.

Comment on lines 37 to +38
const areAllRunsSelected =
savedRuns?.length > 0 && savedRuns?.length === selectedRuns?.length;
savedRuns?.length > 0 && savedRuns?.length === selectedSavedRuns?.length;
Copy link
Member

Choose a reason for hiding this comment

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

The ?. operator needs to be used with considerable care.

For instance, while savedRuns?.length > 0 will "do the right thing" if length is not present, the inverse, savedRuns?.length <= 0, probably won't (that is, they both return false if length is not present, which might be counterintuitive).

Also, we can (and should) use . instead of ?. in the second reference to savedRuns.length, because we know that the field is present if we get past the first test.

So, this code should do an explicit test for the presence of the field, and then test it's value:

    savedRuns?.length && savedRuns.length > 0 && savedRuns.length === selectedSavedRuns?.length;

However, if saved?.length is true-y, then I'm pretty sure that it is greater than zero, so we can omit the middle comparison.

@@ -107,6 +109,7 @@ const SavedRunsComponent = () => {
selectAllRuns(isSelecting),
isSelected: areAllRunsSelected,
}}
style={{ borderTop: "1px solid #d2d2d2" }}
Copy link
Member

Choose a reason for hiding this comment

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

Just out of curiosity, why are we specifying the style in-line instead of putting it in the .less file?

Comment on lines +36 to +37
{item?.message &&
item?.message.split("\n").map((i, key) => {
Copy link
Member

Choose a reason for hiding this comment

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

We don't need (and therefore shouldn't use) the ?. operator in the second reference.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Dashboard Of and relating to the Dashboard GUI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants