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/minor bugs #1938

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

Fix/minor bugs #1938

wants to merge 2 commits into from

Conversation

noahonyejese
Copy link
Contributor

This PR:

  • This PR adds a getTextHeight function
  • And slightly improves AxisLabel positioning

cc: @bprusinowski

@noahonyejese noahonyejese self-assigned this Dec 10, 2024
Copy link

vercel bot commented Dec 10, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
visualization-tool ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 10, 2024 9:44am

Copy link
Collaborator

@bprusinowski bprusinowski left a comment

Choose a reason for hiding this comment

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

Hey @noahonyejese, thanks for changing this! I think if we tweak the logic to use actual chartWidth (or chartWidth / 2 where applicable), the values should be correct without a need for additional calculations.

If they still are not, we'd need to make sure we pass correct line height, font size, etc to the function – here maybe there would be a way to connect this to MUI typography variants somehow, if the text elements are in fact MUI components 👍

app/utils/get-text-width.ts Show resolved Hide resolved
app/charts/combo/axis-height-linear-dual.tsx Show resolved Hide resolved
Comment on lines 78 to 82
(isOverlapping
? axisLabelFontSize * Math.ceil(overlapAmount) + TITLE_VPADDING
: axisLabelFontSize + TITLE_VPADDING) *
? axisLabelHeight * Math.ceil(overlapAmount) + TITLE_VPADDING
: axisLabelHeight + TITLE_VPADDING) *
2 +
TOP_MARGIN
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

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

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

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,
  };
};

app/utils/use-axis-title-adjustments.ts Show resolved Hide resolved
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.

2 participants