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

feat: add gaps between panes #389

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

johnwalley
Copy link
Owner

No description provided.

@johnwalley johnwalley linked an issue Aug 7, 2022 that may be closed by this pull request
2 tasks
@netlify
Copy link

netlify bot commented Aug 7, 2022

Deploy Preview for allotment-storybook ready!

Name Link
🔨 Latest commit e3c573a
🔍 Latest deploy log https://app.netlify.com/sites/allotment-storybook/deploys/62f01fa10e71a90009f3505f
😎 Deploy Preview https://deploy-preview-389--allotment-storybook.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@netlify
Copy link

netlify bot commented Aug 7, 2022

Deploy Preview for allotment-website canceled.

Name Link
🔨 Latest commit e3c573a
🔍 Latest deploy log https://app.netlify.com/sites/allotment-website/deploys/62f01fa111c2d6000a53a24d

Comment on lines +460 to +464
useEffect(() => {
if (gap) {
document.documentElement.style.setProperty("--gap-size", gap + "px");
}
}, [gap]);

Choose a reason for hiding this comment

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

If gap changed from say 5 to 0, this wouldn't change the --gap-size back to 0. Maybe prefer a nullish check? Also I think for effects that modify style, useLayoutEffect is preferred because it runs before paint.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Agreed on both points. I've not progressed this PR because it doesn't play nice with the 'real' dimensions of the panes. If you ask for a minSize of 10 then it won't account for the gap size. Which is a long-winded way of saying I need to go in and change the guts of how allotment works. Which I want to do but don't have the time right now.

The gap === 0 point is a really good one though. Just the sort of thing a code review would catch 😁

Choose a reason for hiding this comment

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

It would be really cool to implement this feature. I need a gap between panels in my designs. Do you have any plans to implement it?

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.

Add gaps between panes
3 participants