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

feat(core): create heatmap chart #1200

Merged
merged 74 commits into from
Dec 24, 2021
Merged

Conversation

Akshat55
Copy link
Collaborator

Updates

  • Create heat map
  • Create custom color scale (Color scale is WIP)
  • Export to all charting libraries

Demo screenshot or recording

image

Review checklist (for reviewers only)

  • Demos all features
  • Documented/annotated
  • Matches UI/UX specs
  • Meets the code style guide
  • Accessible
  • Mobile first (responsive)
  • RTL support (bidirectional text)
  • Performant (limited bloat)

@Akshat55 Akshat55 requested a review from a team as a code owner October 25, 2021 14:21
@Akshat55 Akshat55 requested review from zvonimirfras and Donisius and removed request for a team October 25, 2021 14:21
@Akshat55 Akshat55 changed the title Heatmap feat(core): create heatmap chart Dec 3, 2021
Copy link
Member

@theiliad theiliad left a comment

Choose a reason for hiding this comment

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

Overall I think it looks ok, just a couple of considerations around efficiency and duplication

packages/core/src/styles/graphs/_heatmap.scss Outdated Show resolved Hide resolved
packages/core/src/model/heatmap.ts Outdated Show resolved Hide resolved
packages/core/src/interfaces/enums.ts Outdated Show resolved Hide resolved
*/
title?: string;
type: ColorLegendType;
};
Copy link
Member

Choose a reason for hiding this comment

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

would this add colorLegend to the types for the rest of the charts? is that what we want?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The option currently is ONLY used by Heatmap - if we are going to have future charts that use color legend, it makes sense to put it under the legend option, since it is a legend.

Setting the type or title will not do enable or do anything in other chart types.

Copy link
Member

Choose a reason for hiding this comment

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

Let's (for now) extend this to a HeatmapLegendOptions and only include there, as we haven't figured out how this new legend type will work with other chart types

packages/core/src/interfaces/events.ts Show resolved Hide resolved
packages/core/src/interfaces/enums.ts Outdated Show resolved Hide resolved
Copy link
Member

@theiliad theiliad left a comment

Choose a reason for hiding this comment

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

Just the 2 remaining changes for hover-axis.ts & the legend types requested above

Also @Akshat55 could you pls add the experimental flag to the demos?

@Akshat55
Copy link
Collaborator Author

@theiliad, made them experimental and moved color legend under heatmap options

Copy link
Member

@theiliad theiliad left a comment

Choose a reason for hiding this comment

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

LGTM, tried to look at everything 👍🏻

@theiliad theiliad linked an issue Dec 24, 2021 that may be closed by this pull request
@theiliad theiliad merged commit a55be0f into carbon-design-system:master Dec 24, 2021
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.

Heatmap specs
2 participants