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

fix(heatmap): respect margins and paddings #2577

Open
wants to merge 21 commits into
base: main
Choose a base branch
from

Conversation

mariairiartef
Copy link

@mariairiartef mariairiartef commented Dec 16, 2024

Summary

The margins and paddings are applied correctly for heatmap chart configuration theme.

Screenshot 2025-01-17 at 12 27 35 Screenshot 2025-01-17 at 12 27 47

Small multiples

Screenshot 2025-01-17 at 12 28 52 Screenshot 2025-01-17 at 12 29 02

Brush mask and area

Screen.Recording.2025-01-20.at.16.11.46.mov

Details

Add debug option so when is active elements such as margins, paddings, chart dimensions and chart container dimensions are visible.

Issues

fix #2216

Checklist

  • The proper chart type label has been added (e.g. :xy, :partition)
  • The proper feature labels have been added (e.g. :interactions, :axis)
  • All related issues have been linked (i.e. closes #123, fixes #123)
  • Unit tests have been added or updated to match the most common scenarios
  • The proper documentation and/or storybook story has been added or updated
  • The code has been checked for cross-browser compatibility (Chrome, Firefox, Safari, Edge)

@mariairiartef mariairiartef added bug Something isn't working :heatmap Heatmap/Swimlane chart related issue labels Dec 16, 2024
@mariairiartef mariairiartef self-assigned this Dec 16, 2024
@markov00
Copy link
Member

buildkite test this

Copy link
Author

@mariairiartef mariairiartef Jan 17, 2025

Choose a reason for hiding this comment

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

The methods in the line.ts, rect.ts and text.ts files were extracted from packages/charts/src/chart_types/xy_chart/renderer/canvas/primitives folder. In other to avoid duplicates, we need to use these methods in the xy_chart from the packages/charts/src/renderers/canvas/primitives folder.

As we don't want to increase the number of files, we will do this change in a separate PR.

Copy link
Author

Choose a reason for hiding this comment

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

Created #2586

@mariairiartef mariairiartef marked this pull request as ready for review January 21, 2025 09:04
packages/charts/src/common/colors.tsx Outdated Show resolved Hide resolved
packages/charts/src/common/colors.tsx Outdated Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

You can probably remove these methods and call directly the renderDebugRect passing the right style and dimension

Copy link
Author

@mariairiartef mariairiartef Jan 22, 2025

Choose a reason for hiding this comment

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

if it is not used in any other place we can remove it and call it directly. But I think that creating a method and calling them with its name looks clearer:

() => debug && renderMargins(ctx, chartContainerDimensions, chartMargins),
() => debug && renderPaddings(ctx, chartContainerDimensions, chartDimensions, chartMargins, chartPaddings),
() => debug && renderChartContainerDimensions(ctx, chartContainerDimensions),
() => debug && renderChartDimensions(ctx, chartDimensions),

@markov00 markov00 self-requested a review January 22, 2025 13:47
Comment on lines 15 to 16
const greenSemiTransparent = [...Colors.Green.rgba.slice(0, 3), 0.5] as RgbaTuple;
const lightBlueSemiTransparent = [...Colors.LightBlue.rgba.slice(0, 3), 0.5] as RgbaTuple;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const greenSemiTransparent = [...Colors.Green.rgba.slice(0, 3), 0.5] as RgbaTuple;
const lightBlueSemiTransparent = [...Colors.LightBlue.rgba.slice(0, 3), 0.5] as RgbaTuple;
const greenSemiTransparent = [...Colors.Green.rgba.slice(0, 3), 0.5] satisfies RgbaTuple;
const lightBlueSemiTransparent = [...Colors.LightBlue.rgba.slice(0, 3), 0.5] satisfies RgbaTuple;

Copy link
Member

Choose a reason for hiding this comment

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

You can probably even move these const within each function method. There is no benefit of having that there. These methods are also just for debug purposes so they are not called frequently

Comment on lines 77 to 80
chartContainerDimensions: Dimensions,
chartDimensions: Dimensions,
chartMargins: PerSideDistance,
chartPaddings: PerSideDistance,
Copy link
Member

Choose a reason for hiding this comment

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

nit:

Suggested change
chartContainerDimensions: Dimensions,
chartDimensions: Dimensions,
chartMargins: PerSideDistance,
chartPaddings: PerSideDistance,
container: Dimensions,
chart: Dimensions,
margins: PerSideDistance,
paddings: PerSideDistance,

It could be nice to start cleaning up a bit these naming and reducing redundant labels.

@elastic-datavis
Copy link
Contributor

✅ Successful Deployment (build#4421) - 6fcafae

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working :heatmap Heatmap/Swimlane chart related issue :theme
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Heatmap does not respect theme.chartMargins
2 participants