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

Custom colors for heatmap #253

Merged
merged 10 commits into from
Mar 27, 2020
Merged

Conversation

rahuldeepattri
Copy link
Contributor

@rahuldeepattri rahuldeepattri commented Mar 26, 2020

Added options to select color for heat maps in both gradient and binal mode.

image

image

closes camicroscope/Distro#93

@rahuldeepattri rahuldeepattri changed the title Custom heatmap Custom colors for heatmap Mar 26, 2020
@birm birm requested review from nanli-emory and birm March 26, 2020 15:10
Copy link
Member

@birm birm left a comment

Choose a reason for hiding this comment

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

Great work, just one little nitpick :)

$(container).children().each(function (index,colorDiv) {
rs.push(colorDiv.querySelector('input').value)
});
return rs.reverse();
Copy link
Member

Choose a reason for hiding this comment

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

I'll invite @nanli-emory to comment on this too, but I think it makes more sense to show the lower value heatmap bins first.

Suggested change
return rs.reverse();
return rs;

Copy link
Contributor Author

@rahuldeepattri rahuldeepattri Mar 26, 2020

Choose a reason for hiding this comment

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

Agreed, i used the reverse order because earlier legends were in that order. I have a feeling that we will need to update legend icons code also. I'll check and update 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.

Also, @birm @nanli-emory what should be maximum limit on number of intervals. As of now i have made min 2 and max 5 intervals.

Copy link
Contributor Author

@rahuldeepattri rahuldeepattri Mar 26, 2020

Choose a reason for hiding this comment

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

Corrected order:
image

Copy link
Member

Choose a reason for hiding this comment

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

max 10 intervals

Copy link
Contributor Author

@rahuldeepattri rahuldeepattri Mar 26, 2020

Choose a reason for hiding this comment

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

The logic for gradient should go from lighter colors for less values to more darker colors for higher values, or from blue to red?
image

Above screen shot has gradient from white to blue for lower to higher values. Of course user can change color of each interval. If this isn't appropriate, @nanli-emory please suggest the 10 colors for which gradient should be formed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

waiting for your reply @nanli-emory.

@birm birm self-requested a review March 27, 2020 00:05
Copy link
Member

@birm birm left a comment

Choose a reason for hiding this comment

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

Ah, another nitpick! Otherwise, this looks good to me!

components/heatmapcontrol/heatmapcontrol.js Outdated Show resolved Hide resolved
Copy link
Member

@birm birm left a comment

Choose a reason for hiding this comment

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

LGTM!

@birm birm merged commit e9c4c53 into camicroscope:develop Mar 27, 2020
@birm birm mentioned this pull request Mar 29, 2020
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.

3 participants