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

Heatmap controls #5263

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Heatmap controls #5263

wants to merge 3 commits into from

Conversation

deecay
Copy link
Contributor

@deecay deecay commented Nov 7, 2020

What type of PR is this? (check all applicable)

  • Feature

Description

Adding more control to heatmap. Brick spacingand zRange (zmin and zmax).

image

Related Tickets & Documents

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

kravets-levko
kravets-levko previously approved these changes Nov 8, 2020
@rafawendel
Copy link
Member

Nice features! I'll wait your considerations on the previous comment and then merge. If you're interested, take a look at this, could be a cool addition!

Copy link
Member

@rafawendel rafawendel left a comment

Choose a reason for hiding this comment

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

Just noticed that for some reason updating the zMin by itself doesn't trigger UI responsiveness. @kravets-levko do you know why? zMax works as expected

@guidopetri
Copy link
Collaborator

@deecay , thanks for the PR! We've updated a lot of things now that we're Community-driven so - if you're still interested in getting this merged - would you mind rebasing off master to re-run the CI, as well as updating merge conflicts?

We're trying to clean up our PR todo list, so if you're not interested, that's fine - we'll close the PR in about a week if we don't hear back. If you're interested in reopening the PR afterwards, we would also very much welcome that.

@justinclift
Copy link
Member

Looking over this PR, it seems like the files it was created for are now using TypeScript (eg GeneralSettings.jsxGeneralSettings.tsx), so will require some rework to match.

It doesn't look too complicated though, at least for someone already familiar with TypeScript (not me at this stage). 😄

@codecov
Copy link

codecov bot commented Oct 26, 2023

Codecov Report

Merging #5263 (5b0ba9c) into master (de84c40) will increase coverage by 0.01%.
Report is 1 commits behind head on master.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #5263      +/-   ##
==========================================
+ Coverage   61.89%   61.90%   +0.01%     
==========================================
  Files         158      158              
  Lines       12966    12966              
  Branches     1773     1773              
==========================================
+ Hits         8025     8027       +2     
+ Misses       4666     4664       -2     
  Partials      275      275              

see 1 file with indirect coverage changes

@justinclift justinclift requested review from eradman and removed request for gabrieldutra October 27, 2023 05:42
@justinclift
Copy link
Member

@deecay Looks like you're getting this one ready. Is it at the stage now that someone (@eradman maybe?) should review it?

@deecay
Copy link
Contributor Author

deecay commented Oct 27, 2023

@justinclift
yes, it should be ready now.

@@ -33,6 +33,10 @@ function prepareSeries(series: any, options: any, additionalOptions: any) {
type: "heatmap",
name: "",
colorscale: colorScheme,
xgap: (options.heatmapSpacing || 0) * 0.2,
ygap: (options.heatmapSpacing || 0) * 0.2,
Copy link
Collaborator

@eradman eradman Oct 27, 2023

Choose a reason for hiding this comment

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

We do not need to divide the user input. If the user wants a very faint line they can input 0.2

Copy link
Contributor Author

@deecay deecay Oct 27, 2023

Choose a reason for hiding this comment

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

Done in [fcf6e1b].

@@ -3,6 +3,8 @@ import React, { useMemo } from "react";
import { Section, Select, Checkbox, InputNumber, ContextHelp, Input } from "@/components/visualizations/editor";
import { UpdateOptionsStrategy } from "@/components/visualizations/editor/createTabbedEditor";
import { EditorPropTypes } from "@/visualizations/prop-types";
import * as Grid from "antd/lib/grid";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we avoid using import *? It would be better to see the module properties this line is intended to import

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 5b0ba9c .

@eradman
Copy link
Collaborator

eradman commented Oct 27, 2023

I think this is good to go

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants