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

#1144 Disable node coloring when uploading file with no expression #1153

Merged
merged 5 commits into from
Dec 4, 2024
Merged

#1144 Disable node coloring when uploading file with no expression #1153

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c8c22bb
#1144 Adding code to set node-coloring enable only when having expres…
ntran18 Nov 20, 2024
05cd4bb
#1144 fix previous commit code that didn't work, changing from workbo…
ntran18 Nov 27, 2024
eed34ff
Clean code
ntran18 Nov 27, 2024
e94143a
Rewrite hasExpressionData function to use -some- call
ntran18 Nov 30, 2024
d071741
Remove one hasExpressionData function
ntran18 Dec 4, 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
7 changes: 1 addition & 6 deletions web-client/public/js/graph.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1235,12 +1235,7 @@ export var drawGraph = function (workbook) {
};

const hasExpressionData = sheets => {
for (var property in sheets) {
if (property.match(ENDS_IN_EXPRESSION_REGEXP)) {
return true;
}
}
return false;
return Object.keys(sheets).some(property => property.match(ENDS_IN_EXPRESSION_REGEXP));
Copy link
Owner

Choose a reason for hiding this comment

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

Thanks, this looks good! But it looks like this function is duplicated in update-app.js? (see line 515 below) Should these functions be consolidated into one?

Sorry I didn’t notice this before…

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 didn't notice it either ....

};

if (!$.isEmptyObject(workbook.expression) && hasExpressionData(workbook.expression) &&
Expand Down
4 changes: 2 additions & 2 deletions web-client/public/js/setup-handlers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ export const setupHandlers = grnState => {

const explicitlySetStyle = element => {
const cssStyleDeclarationComputed = window.getComputedStyle(element);
const computedStyleObj = {};
const computedStyleObj = {};


for (let i = 0; i < cssStyleDeclarationComputed.length; i++) {
Expand All @@ -134,7 +134,7 @@ export const setupHandlers = grnState => {
}

if (computedStyleObj) {
Object.assign(element.style, computedStyleObj)
Object.assign(element.style, computedStyleObj);
}
};

Expand Down
22 changes: 12 additions & 10 deletions web-client/public/js/update-app.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -512,6 +512,15 @@ const toggleLayout = (on, off) => {
}
};

const hasExpressionData = (sheets) => {
for (var property in sheets) {
if (property.match(ENDS_IN_EXPRESSION_REGEXP)) {
return true;
}
}
return false;
};
Copy link
Owner

Choose a reason for hiding this comment

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

This isn’t functionally incorrect, but along the lines of more current idioms when processing lists or objects, see if this can be written as a some call? https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/some

Using some will make the code look something like this…

Suggested change
const hasExpressionData = (sheets) => {
for (var property in sheets) {
if (property.match(ENDS_IN_EXPRESSION_REGEXP)) {
return true;
}
}
return false;
};
const hasExpressionData = (sheets) => {
return (/*expression with sheets*/).some(
value => (/*expression with value */).match(ENDS_IN_EXPRESSION_REGEXP)
)
};

…which to me sounds more concise, and objectively does not use side effects as well as easier for an optimizer to parallelize

I realize sheets isn’t an array, but there may be ways to take advantage of the simpler expressiveness of some vs. having a return true with a return false at the end (that’s why I have some placeholder /* */ comments in there instead of specific code; there may be other places too)

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 just modified it to use some call instead.

Copy link
Owner

Choose a reason for hiding this comment

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

You certainly modified it in graph.js, but does that mean that lines 515–522 above are duplicated?


const updatetoForceGraph = () => {};

const updatetoGridLayout = () => {};
Expand Down Expand Up @@ -552,7 +561,9 @@ const updateModeViews = () =>{
};

const checkWorkbookModeSettings = () => {
if (grnState.mode === NETWORK_PPI_MODE) {
const hasExpression = hasExpressionData(grnState.workbook.expression);

if (grnState.mode === NETWORK_PPI_MODE || !hasExpression) {
grnState.nodeColoring.nodeColoringEnabled = false;
grnState.nodeColoring.showMenu = true;
grnState.colorOptimal = false;
Expand Down Expand Up @@ -598,15 +609,6 @@ const shortenExpressionSheetName = (name) => {
(name.slice(0, MAX_NUM_CHARACTERS_DROPDOWN) + "...") : name;
};

const hasExpressionData = (sheets) => {
for (var property in sheets) {
if (property.match(ENDS_IN_EXPRESSION_REGEXP)) {
return true;
}
}
return false;
};

const updateSpeciesMenu = () => {
$(SPECIES_DISPLAY).val(grnState.genePageData.species);
$(SPECIES_BUTTON_CRESS + " span").removeClass("glyphicon-ok");
Expand Down