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

[ui] Refactor SplitPanelContainer #20909

Merged
merged 1 commit into from
Mar 29, 2024
Merged

Conversation

hellendag
Copy link
Member

@hellendag hellendag commented Mar 29, 2024

Summary & Motivation

I set out to add a minimum size for the second panel in a SplitPanelContainer, and I fell into a refactoring rabbit hole.

  • Add a minimum size (width/height) for the second pane, as originally planned.
  • Rewrite into functional components, with an imperative handle on SplitPanelContainer to replicate its imperative API.
  • Set a minimum for a few different panes around the app (global asset graph, run log pane)
  • Fix up the expand/collapse panel on launchpad config, which has some tricky ref usage.
  • Tidy up a few other things that I found hard to follow or confusing.

Note that this may lead to some panel size allocations being slightly different from before, in cases where we have a minimum size for the second panel. For instance, if I set a minimum of 400px for the second panel, and the first panel is set to occupy 75% of the container, that 75% will now apply to the available size minus 400px, instead of 75% of the full container. FWIW, I don't think this will be a noticeable change for most people.

Screen.Recording.2024-03-29.at.12.07.49.mov

How I Tested These Changes

View launchpad, run page, global asset graph. Drag vertical and horizontal split panels, verify correct behavior. Verify that second-panel minimums are respected.

Test split panel contain

Copy link
Member Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @hellendag and the rest of your teammates on Graphite Graphite

Copy link

Deploy preview for dagit-storybook ready!

✅ Preview
https://dagit-storybook-pwzo95ou7-elementl.vercel.app
https://dish-asset-sidebar-min-width.components-storybook.dagster-docs.io

Built with commit 3e9b43c.
This pull request is being automatically deployed with vercel-action

Copy link

Deploy preview for dagit-core-storybook ready!

✅ Preview
https://dagit-core-storybook-5o02d738o-elementl.vercel.app
https://dish-asset-sidebar-min-width.core-storybook.dagster-docs.io

Built with commit 3e9b43c.
This pull request is being automatically deployed with vercel-action

@hellendag hellendag requested review from bengotow and salazarm March 29, 2024 17:18
@hellendag hellendag marked this pull request as ready for review March 29, 2024 17:18
const onChangeSize = useCallback(
(newValue: number) => {
window.localStorage.setItem(key, `${newValue}`);
setTrigger((current) => (current ? 0 : 1));
Copy link
Member Author

Choose a reason for hiding this comment

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

We're already tracking the size value in localStorage, so we don't need to duplicate that in local state.

firstPaneStyles.height = `calc(${firstSize}% - ${DIVIDER_THICKNESS}px)`;
firstPaneStyles.height = `calc(${firstSize / 100} * (100% - ${
DIVIDER_THICKNESS + (second ? secondMinSize : 0)
}px))`;
Copy link
Member Author

Choose a reason for hiding this comment

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

The noteworthy calculation change to accommodate minimum size on the second panel.

? ((event.clientX - parentRect.left) * 100) /
(parentRect.width - secondMinSize - DIVIDER_THICKNESS)
: ((event.clientY - parentRect.top) * 100) /
(parentRect.height - secondMinSize - DIVIDER_THICKNESS);
Copy link
Member Author

Choose a reason for hiding this comment

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

Ensures that the divider remains in the correct position with respect to the minimums applied.

@hellendag hellendag merged commit c1740a1 into master Mar 29, 2024
3 checks passed
@hellendag hellendag deleted the dish/asset-sidebar-min-width branch March 29, 2024 17:28
salazarm added a commit that referenced this pull request Apr 11, 2024
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