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: support resize #119

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

feat: support resize #119

wants to merge 5 commits into from

Conversation

hyoban
Copy link
Contributor

@hyoban hyoban commented Jan 13, 2024

ScreenShot.2024-01-13.21.50.29.mp4

Copy link

codesandbox-ci bot commented Jan 13, 2024

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 55b71bb:

Sandbox Source
React Configuration
React TypeScript Configuration

@hyoban hyoban marked this pull request as ready for review January 13, 2024 12:50
@hyoban
Copy link
Contributor Author

hyoban commented Jan 13, 2024

It looks like react-moveable cannot be used with jsdom

@hyoban hyoban changed the title feat: support drag and resize feat: support resize Jan 13, 2024
Copy link
Member

@arjunvegda arjunvegda left a comment

Choose a reason for hiding this comment

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

This is very cool! Appreciate you working on this. Left a few comments, I'll try to pull this locally and test it out too.

import { TabsHeader } from './components/TabsHeader';
import { TimeTravel } from './components/TimeTravel';
import { shellStyles } from './styles';

function areWeTestingWithJest() {
return process.env.JEST_WORKER_ID !== undefined;
Copy link
Member

Choose a reason for hiding this comment

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

Is this just a problem with jest? I wonder react-movable's issue is limited to jest and not other testing libraries like vitest 🤔

And what's the issue you're running into? I'm not a huge fan of having this logic in the library. Is there an alternate library that we could explore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is related to jsdom

jsdom/jsdom#1435

I just found out that if you just use resize you don't have a problem

Copy link
Member

Choose a reason for hiding this comment

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

Got it! so the issue would likely persist for users mounting Jotai DevTools in the tests? Do we have any workarounds, maybe a polyfill or finding a different library?

Comment on lines +44 to +57
sx={(theme) => {
return {
...(typeof shellStyles === 'function'
? shellStyles(theme)
: shellStyles),
transform: shellStyle.transform,
};
}}
mah={shellStyleDefaults.maxHeight}
maw={shellStyleDefaults.maxWidth}
mih={shellStyleDefaults.minHeight}
miw={shellStyleDefaults.minWidth}
h={shellStyle.height}
w={shellStyle.width}
Copy link
Member

Choose a reason for hiding this comment

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

This feels a bit hacky, would it be a good idea to have two separate variables, one holds the shell styles, and the other holds the styles set by Movable. And we merge those two here 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean merging shellStyleDefaults and shellStyle?

Copy link
Member

Choose a reason for hiding this comment

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

Yes! That and shellStyles being a function or object. Ideally we don't want the type to be polymorphic at runtime.

Perhaps something like this?

const mergedTabStyles = (theme) => {
  return {
    ...shellStyleDefaults,
    ...shellStyles(theme),
    ...movableStyles,
  };
};

// ... 
sx={mergedTabStyles}
// ... 

Comment on lines +100 to +103
onResize={(e) => {
e.target.style.width = `${e.width}px`;
e.target.style.height = `${e.height}px`;
e.target.style.transform = e.drag.transform;
Copy link
Member

Choose a reason for hiding this comment

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

Is this our only way? do you know if <Movable/> accepts a style/classname?

import { TabsHeader } from './components/TabsHeader';
import { TimeTravel } from './components/TimeTravel';
import { shellStyles } from './styles';

function areWeTestingWithJest() {
return process.env.JEST_WORKER_ID !== undefined;
Copy link
Member

Choose a reason for hiding this comment

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

Got it! so the issue would likely persist for users mounting Jotai DevTools in the tests? Do we have any workarounds, maybe a polyfill or finding a different library?

@@ -45,26 +45,6 @@ describe('DevTools - basic', () => {
});
});

it('should resize the devtools upon dragging the resize bar', async () => {
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to test the drag drop + resize functionality?

@@ -17,58 +18,97 @@ export const Shell = () => {
const [selectedShellTab, setSelectedShellTab] = useSelectedShellTab();
Copy link
Member

Choose a reason for hiding this comment

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

Should we add prop to set default panel position? like you did for the trigger buttont?

Maybe something like? panelPosition?: "top" | "right" | "bottom" | "left" 🤔

@jigglypop
Copy link

so usefull

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.

None yet

3 participants