Skip to content

WIP: Fixed Demo Indicators out of sync bug and updated export modal and functionality#985

Merged
dondi merged 23 commits intobetafrom
Onariaginosa
Oct 19, 2022
Merged

WIP: Fixed Demo Indicators out of sync bug and updated export modal and functionality#985
dondi merged 23 commits intobetafrom
Onariaginosa

Conversation

@Onariaginosa
Copy link
Collaborator

@Onariaginosa Onariaginosa commented Sep 14, 2022

#976 Fixed this issue. The demo sidebar dropdown works as expected now.
#978 UI fixes and beginnings of the new excel export implementation.
#990 Fixed this issue. GRNsight properly handles when a database generated network does not have any regulatory connections.
#977 @ahmad00m also fixed the node coloring menu inactivated issue in this pr
#972 Fixed this issue. Edges are now counted correctly in the custom workbook title
#961 Documentation for loading database scripts completed

Copy link
Owner

@dondi dondi left a comment

Choose a reason for hiding this comment

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

Just a couple of tweaks and I think this will work!

@Onariaginosa Onariaginosa changed the title Fixed Demo Indicators out of sync bug and updated export modal WIP: Fixed Demo Indicators out of sync bug and updated export modal and functionality Oct 12, 2022
Copy link
Owner

@dondi dondi left a comment

Choose a reason for hiding this comment

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

Overall looks pretty good except for just a couple of code style fine points. Once those are changed I think this can go…

Comment on lines +149 to +150
let date = new Date(source.substring(i + 1));
date = getFormattedDateTime(date);
Copy link
Owner

Choose a reason for hiding this comment

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

Hmmm this is a little sketchy. Starting the date variable out as a Date and then replacing it with its string…JavaScript lets you do this but other languages won’t, and ideally we don’t get into the habit of doing this. If we create a new value, we should put that in a new variable

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I changed this as requested

// these are part of the "meta" property
} else if (TWO_COL_SHEET_NAMES.includes(sheet.name)) {
output["test"][sheet.name] = parseTwoColumnSheet(sheet);
output["twoColumnSheets"][sheet.name] = parseTwoColumnSheet(sheet);
Copy link
Owner

Choose a reason for hiding this comment

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

Any reason why this isn’t output.twoColumnSheets? (it’s written that way elsewhere)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I changed it to to output.twoColumnSheets. It must of been residual from fixing linter errors.

Copy link
Owner

@dondi dondi left a comment

Choose a reason for hiding this comment

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

Almost there! Just one little tweak. Values reflected here based on the latest capabilities of JavaScript:

  • Use const as much as possible
  • Declare variables at the deepest scope in which they’re needed

Co-authored-by: dondi <dondi@lmu.edu>
Copy link
Owner

@dondi dondi left a comment

Choose a reason for hiding this comment

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

LGTM 🤗

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