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 position of overflowing annotations by refactoring VisCanvas #1467

Merged
merged 2 commits into from
Aug 21, 2023

Conversation

axelboc
Copy link
Contributor

@axelboc axelboc commented Aug 10, 2023

As mentioned in #1466, annotations with overflowCanvas do not work as expected: they are positioned in relation to canvasWrapper, the root element rendered by VisCanvas, which defines padding for the axes.

In this PR, I remedy this problem by refactoring VisCanvas once again in order to have a canvasWrapper element with the same size as the canvas that, unlike r3fRoot, does not hide overflow.

The root element rendered by VisCanvas is now called visCanvas (this may change in later refactors) and no longer has any padding. Instead, it takes on the role of axisSystem by setting up the CSS grid that lays everything out:

  • canvasWrapper is now a child of visCanvas and positioned with grid-area.
  • The axes and the grids are rendered as direct children of visCanvas and also positioned with grid-area.

So instead of:

.canvasWrapper (padding)
  .r3fRoot (hidden overflow)
    canvas
    .svgOverlay
    .floatingToolbar
  .axisSystem
    .axis
    .grid
    .axis
    .grid

... we now have:

.visCanvas (no padding)
  .canvasWrapper (visible overflow)
    .r3fRoot (hidden overflow)
      canvas
      .svgOverlay
      .floatingToolbar
  .axis
  .grid
  .axis
  .grid

I had tried similar refactors before but always ran into overflow issues. The key that unlocked it this time is the use of minmax(0, 1fr) instead of 1fr for sizing the grid area reserved for the canvas. This has a similar effect to min-width: 0 and min-height: 0 when using Flexbox. See https://css-tricks.com/preventing-a-grid-blowout/

Another important reason this layout works is because CSS Grid allows positioning multiple grid items into the same grid area. The items then behave as if they were positioned absolutely on top of one another. In our case, the axis grids are positioned in the same CSS Grid area as canvasWrapper. I feel like this will enable further refactors.

@axelboc axelboc changed the base branch from html-portal to main August 10, 2023 11:14
@axelboc
Copy link
Contributor Author

axelboc commented Aug 10, 2023

/approve

Copy link
Member

@loichuder loichuder left a comment

Choose a reason for hiding this comment

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

👍

packages/lib/src/vis/shared/AxisSystem.tsx Show resolved Hide resolved
packages/lib/src/vis/shared/VisCanvas.tsx Show resolved Hide resolved
@axelboc axelboc merged commit f5540e9 into main Aug 21, 2023
8 checks passed
@axelboc axelboc deleted the fix-annoations branch August 21, 2023 13:33
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