-
Notifications
You must be signed in to change notification settings - Fork 33
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
feat: support resize #119
Conversation
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. |
It looks like |
There was a problem hiding this 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; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
I just found out that if you just use resize
you don't have a problem
There was a problem hiding this comment.
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?
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} |
There was a problem hiding this comment.
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 🤔
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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}
// ...
onResize={(e) => { | ||
e.target.style.width = `${e.width}px`; | ||
e.target.style.height = `${e.height}px`; | ||
e.target.style.transform = e.drag.transform; |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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?
__tests__/devtools/basic.test.tsx
Outdated
@@ -45,26 +45,6 @@ describe('DevTools - basic', () => { | |||
}); | |||
}); | |||
|
|||
it('should resize the devtools upon dragging the resize bar', async () => { |
There was a problem hiding this comment.
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(); |
There was a problem hiding this comment.
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"
🤔
so usefull |
ScreenShot.2024-01-13.21.50.29.mp4