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

Fix/minor bugs #1938

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
+60 −11
Open

Fix/minor bugs #1938

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4f59a1f
feat: Added new getTextHeight function
noahonyejese Dec 10, 2024
fe17af3
fix: replaced axisLabelFontSize w/ axisLabelHeight
noahonyejese Dec 10, 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
9 changes: 6 additions & 3 deletions app/charts/combo/axis-height-linear-dual.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import { useChartState } from "@/charts/shared/chart-state";
import { useChartTheme } from "@/charts/shared/use-chart-theme";
import { OpenMetadataPanelWrapper } from "@/components/metadata-panel";
import { theme } from "@/themes/federal";
import { getTextWidth } from "@/utils/get-text-width";
import { getTextHeight, getTextWidth } from "@/utils/get-text-width";
import { useAxisTitleAdjustments } from "@/utils/use-axis-title-adjustments";

import { TITLE_VPADDING } from "./combo-line-container";
Expand Down Expand Up @@ -65,6 +65,9 @@ export const AxisHeightLinearDual = (props: AxisHeightLinearDualProps) => {
TITLE_HPADDING * (isOverlapping ? Math.floor(overlapAmount) : 2);

const titleWidth = isOverlapping ? axisTitleWidth / overlapAmount : "auto";
const axisLabelHeight = getTextHeight(axisTitle, {
fontSize: axisLabelFontSize,
});
noahonyejese marked this conversation as resolved.
Show resolved Hide resolved

return (
<>
Expand All @@ -73,8 +76,8 @@ export const AxisHeightLinearDual = (props: AxisHeightLinearDualProps) => {
width={axisTitleWidth + TITLE_HPADDING * 2}
height={
(isOverlapping
? axisLabelFontSize * Math.ceil(overlapAmount) + TITLE_VPADDING
: axisLabelFontSize + TITLE_VPADDING) *
? axisLabelHeight * Math.ceil(overlapAmount) + TITLE_VPADDING
: axisLabelHeight + TITLE_VPADDING) *
2 +
TOP_MARGIN
Comment on lines 78 to 82
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we add width as above (chartWidth / 2) to getTextHeight, I am pretty sure we don't need this logic :) The goal of getTextHeight was to get "correct" height, without a need to multiply or divide things anymore 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bprusinowski I will try to refactor it, I have an Idea...

Copy link
Contributor Author

@noahonyejese noahonyejese Dec 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

image

The problem is stuff like this. This counts as 3 lines I don't why, but (#) doesnt count as a line for some reason. if I log the value it says. 3 lines.

Copy link
Collaborator

@bprusinowski bprusinowski Dec 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

But with this we don't need to know the number of lines, as the text would naturally wrap inside the measurement container so we get correct height back, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the text is within a foreignObject which needs the exact height of all lines combined to ensure correct amount of space for the span element.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, but here I was thinking about getTextHeight method - the flow would be:

  • get width of the label (half of chart width, potentially with some padding),
  • get text height, passing the width to the function, so that the text can naturally wrap inside the measurement container,
  • set foreignObject height using the text height.

I don't see a reason to get number of lines of the text, did I misunderstand something? 🥹

Copy link
Contributor Author

@noahonyejese noahonyejese Dec 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

aha yeah, no I meant that yes the getTextHeight returns the wrong number, if its 4 lines it returns f.e 45 instead of f.e 60, or in the image specifically it returns 45 for the one on the right while the one on the left is 30 even tho it should technically be 60, it always takes the max height of both labels and then uses the largest one, however this doesn't seem to work properly. @bprusinowski

here is the main logic:

export const useDualAxisTitleAdjustments = ({
  leftAxisTitle,
  rightAxisTitle,
  containerWidth,
  fontSize = TICK_FONT_SIZE,
}: useDualAxisTitleAdjustmentsProps): AxisTitleAdjustments => {
  const axisTitleWidthLeft =
    getTextWidth(leftAxisTitle, { fontSize }) + TICK_PADDING;
  const axisTitleWidthRight =
    getTextWidth(rightAxisTitle, { fontSize }) + TICK_PADDING;

  const axisTitleHeightLeft = getTextHeight(leftAxisTitle, {
    fontSize,
    width: containerWidth / 2,
    lineHeight: "15px",
  });

  const axisTitleHeightRight = getTextHeight(rightAxisTitle, {
    fontSize,
    width: containerWidth / 2,
    lineHeight: "15px",
  });

  const maxAxisLabelHeight = Math.max(
    axisTitleHeightRight,
    axisTitleHeightLeft
  );

  const isOverlapping =
    axisTitleWidthLeft + axisTitleWidthRight > containerWidth;
  const overlapAmount =
    (axisTitleWidthLeft + axisTitleWidthRight) / containerWidth;

  const axisTitleAdjustment = maxAxisLabelHeight + TITLE_VPADDING * 2;

  console.log(axisTitleHeightLeft, axisTitleHeightRight);

  const topMarginAxisTitleAdjustment = 60 + axisTitleAdjustment;

  return {
    axisLabelHeight: maxAxisLabelHeight,
    axisTitleAdjustment,
    topMarginAxisTitleAdjustment,
    isOverlapping,
    overlapAmount,
  };
};

}
Expand Down
8 changes: 6 additions & 2 deletions app/charts/combo/combo-line-container.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { ReactNode } from "react";

import { getTextWidth } from "@/utils/get-text-width";
import { getTextHeight, getTextWidth } from "@/utils/get-text-width";

import { TICK_PADDING } from "../shared/axis-height-linear";
import { useChartState } from "../shared/chart-state";
Expand All @@ -26,8 +26,12 @@ export const ComboLineContainer = ({ children }: { children: ReactNode }) => {
const overLappingTitles =
axisTitleWidth + otherAxisTitleWidth > bounds.chartWidth;

const axisLabelHeight = getTextHeight(axisTitle, {
fontSize: axisLabelFontSize,
});

const yAdjustment = overLappingTitles
? (axisLabelFontSize + TITLE_VPADDING) * 2
? (axisLabelHeight + TITLE_VPADDING) * 2
: 0;
return <g transform={`translate(0, ${yAdjustment})`}>{children}</g>;
};
8 changes: 6 additions & 2 deletions app/charts/combo/combo-line-dual-state.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ import { InteractionProvider } from "@/charts/shared/use-interaction";
import { ComboLineDualConfig } from "@/configurator";
import { Observation } from "@/domain/data";
import { truthy } from "@/domain/types";
import { getTextWidth } from "@/utils/get-text-width";
import { getTextHeight, getTextWidth } from "@/utils/get-text-width";
import { useAxisTitleAdjustments } from "@/utils/use-axis-title-adjustments";
import { useIsMobile } from "@/utils/use-is-mobile";

Expand Down Expand Up @@ -244,8 +244,12 @@ const ComboLineDualChartProvider = (
const overLappingTitles =
axisTitleWidth + otherAxisTitleWidth > bounds.chartWidth;

const axisLabelHeight = getTextHeight(axisTitle, {
fontSize: axisLabelFontSize,
});

if (overLappingTitles) {
bounds.height += axisLabelFontSize + TITLE_VPADDING; // Add space for the legend if titles are overlapping
bounds.height += axisLabelHeight + TITLE_VPADDING; // Add space for the legend if titles are overlapping
}

return (
Expand Down
36 changes: 36 additions & 0 deletions app/utils/get-text-width.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,3 +14,39 @@ export const getTextWidth = (text: string, options: { fontSize: number }) => {

return ctx.measureText(text).width;
};

export const getTextHeight = (
text: string,
options: {
fontSize: number;
width?: number;
lineHeight?: number | string;
}
) => {
const measureDiv = document.createElement("div");

measureDiv.style.font = `${options.fontSize}px ${fontFamily}`;
measureDiv.style.position = "absolute";
measureDiv.style.visibility = "hidden";
measureDiv.style.lineHeight = options.lineHeight
? typeof options.lineHeight === "number"
? `${options.lineHeight}px`
: options.lineHeight
noahonyejese marked this conversation as resolved.
Show resolved Hide resolved
: "normal";

if (options.width) {
measureDiv.style.width = `${options.width}px`;
} else {
measureDiv.style.whiteSpace = "nowrap";
}

measureDiv.textContent = text;

document.body.appendChild(measureDiv);

const height = measureDiv.offsetHeight;

document.body.removeChild(measureDiv);

return height;
};
10 changes: 6 additions & 4 deletions app/utils/use-axis-title-adjustments.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { TITLE_VPADDING } from "@/charts/combo/combo-line-container";
import { TICK_PADDING } from "@/charts/shared/axis-height-linear";
import { TICK_FONT_SIZE } from "@/charts/shared/use-chart-theme";
import { getTextWidth } from "@/utils/get-text-width";
import { getTextHeight, getTextWidth } from "@/utils/get-text-width";

export interface AxisTitleAdjustments {
axisTitleAdjustment: number;
Expand Down Expand Up @@ -33,12 +33,14 @@ export const useAxisTitleAdjustments = ({
const overlapAmount =
(axisTitleWidthLeft + axisTitleWidthRight) / containerWidth;

const axisLabelHeight = getTextHeight(leftAxisTitle, { fontSize });
noahonyejese marked this conversation as resolved.
Show resolved Hide resolved

const axisTitleAdjustment =
(isOverlapping
? fontSize * Math.ceil(overlapAmount)
: fontSize + TITLE_VPADDING) *
? axisLabelHeight * Math.ceil(overlapAmount)
: axisLabelHeight + TITLE_VPADDING) *
2 -
fontSize * 2;
axisLabelHeight * 2;

const topMarginAxisTitleAdjustment = 60 + axisTitleAdjustment;

Expand Down
Loading