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
Add general purpose draggable panels #140
Merged
adnauseum
merged 41 commits into
main
from
gh-issue-420-add-general-purpose-draggable-panel
Feb 6, 2023
Merged
Changes from all commits
Commits
Show all changes
41 commits
Select commit
Hold shift + click to select a range
c31570e
Add .vscode directory
adnauseum b800b94
Add watch script for testing
adnauseum 19fab78
Add stub tests
adnauseum 27955f7
Update .gitignore with .vscode dir
adnauseum c24cd52
Add tests
adnauseum a997cf1
Remove somebody's vscode settings
adnauseum 3a938ac
Consolidate test cases
adnauseum 37eb73b
Change some names
adnauseum 9b1a2af
Remove typo
adnauseum ff440ca
Add test for onDragComplete
adnauseum bd2bc58
Rename state var name
adnauseum 5bf67a8
Rename onClose to onPanelDismiss
adnauseum a18942d
Ensure onDragComplete is included in prop examples
adnauseum 9c4692b
Add type definition for panel props
adnauseum 8013339
Well, the tests are green
adnauseum 89bf237
Consolidate test cases
adnauseum 26b0679
Add comment
adnauseum 6b449e1
Test add transform assertion
adnauseum 262fa73
Add some styles
adnauseum 9f39adc
Add initial panel height and width
adnauseum 0103c2c
Change selector back to visibility
adnauseum acfe7b4
Complete styling
adnauseum 7d7f1ec
Add prop to toggle confining draggables to their parent element
adnauseum 8f7bd54
Add border-radius to match other components
adnauseum b195a49
Add comment
adnauseum f9a9db0
Inline styles
adnauseum e566e09
Format comment
adnauseum 2b7a15c
Change variable name
adnauseum 503ad1c
Refactor conditional
adnauseum 1910dbe
Add test for stacking after drag
adnauseum 7778671
Not going to add a parent container
adnauseum a609abd
Add tests for layering
adnauseum 73b5a61
Refactor test
adnauseum 5c73ca4
Implement code to make tests work
adnauseum bc58e39
Add comments for myself
adnauseum 087d1fa
Add button text to dismiss button for testing and accessibility
adnauseum 203a51f
Tweak functionality and style
adnauseum 5237bee
Add support for resizing and emiting resize events
adnauseum a49daed
Add salty comment
adnauseum e718f5e
Remove unused imports
adnauseum 6cee71b
Remove needless mock
adnauseum File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,4 +2,5 @@ node_modules | |
dist | ||
.DS_Store | ||
build-storybook.log | ||
/coverage | ||
/coverage | ||
.vscode/* |
This file was deleted.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,2 +1,8 @@ | ||
import "@testing-library/jest-dom"; // This wires up React Testing Library and Jest. | ||
import "regenerator-runtime/runtime"; // Without this you'll this error when running tests: ReferenceError: regeneratorRuntime is not defined. | ||
import "regenerator-runtime/runtime"; // Without this you'll this error when running tests: ReferenceError: regeneratorRuntime is not defined. | ||
import "@testing-library/jest-dom/extend-expect"; | ||
|
||
// So this piece of tragedy is due to yet another jsdom limitation. | ||
import resizeObserverPolyfill from "resize-observer-polyfill"; | ||
// eslint-disable-next-line no-undef | ||
global.ResizeObserver = resizeObserverPolyfill; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
213 changes: 213 additions & 0 deletions
213
src/components/containers/DraggablePanel/DraggablePanel.test.tsx
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,213 @@ | ||
import { fireEvent, render, screen } from "@testing-library/react"; | ||
import { useState } from "react"; | ||
import { DraggableProps } from "react-draggable"; | ||
import { | ||
DraggablePanel, | ||
DraggablePanelCoordinatePair, | ||
DraggablePanelProps, | ||
} from "./DraggablePanel"; | ||
|
||
describe("Draggable Panels", () => { | ||
test("dragging a panel changes where it lives.", () => { | ||
const defaultPosition: DraggablePanelCoordinatePair = { x: 0, y: 0 }; | ||
const panelTitle = "Study Filters Panel"; | ||
const handleOnDragComplete = jest.fn(); | ||
render( | ||
<DraggablePanel | ||
defaultPosition={defaultPosition} | ||
isOpen | ||
onDragComplete={handleOnDragComplete} | ||
onPanelDismiss={() => {}} | ||
panelTitle={panelTitle} | ||
showPanelTitle | ||
> | ||
<p>Panel contents</p> | ||
</DraggablePanel> | ||
); | ||
const panelDragHandle = screen.getByText(`Close ${panelTitle}`); | ||
|
||
const destinationCoordinates = { clientX: 73, clientY: 22 }; | ||
|
||
drag(panelDragHandle, destinationCoordinates); | ||
|
||
/** | ||
* I really don't like assert on implementation details. If we change React dragging librbaries, | ||
* this assertion could break and raise a false positive. That said, jsdom doesn't render layouts | ||
* like a legit browser so we're left with this and data-testids. The data-testid is nice because | ||
* at least we're in control of that so we can make sure that doesn't change if we swap dragging | ||
* providers. See conversations like: https://softwareengineering.stackexchange.com/questions/234024/unit-testing-behaviours-without-coupling-to-implementation-details | ||
*/ | ||
const panelFromDataTestId = screen.getByTestId(`${panelTitle} dragged`); | ||
expect(panelFromDataTestId.style.transform).toEqual( | ||
`translate(${destinationCoordinates.clientX}px,${destinationCoordinates.clientY}px)` | ||
); | ||
|
||
expect(panelFromDataTestId).toBeTruthy(); | ||
expect(handleOnDragComplete).toHaveBeenCalled(); | ||
}); | ||
|
||
test("you can open and close panels", async () => { | ||
const defaultPosition = { x: 50, y: 50 }; | ||
|
||
function ToggleButtonAndDraggablePanel() { | ||
const [panelIsOpen, setPanelIsOpen] = useState(true); | ||
return ( | ||
<> | ||
<button onClick={() => setPanelIsOpen((isOpen) => !isOpen)}> | ||
Toggle Filters Panel | ||
</button> | ||
<DraggablePanel | ||
defaultPosition={defaultPosition} | ||
isOpen={panelIsOpen} | ||
panelTitle="My Filters" | ||
onDragComplete={() => {}} | ||
onPanelDismiss={() => setPanelIsOpen(false)} | ||
showPanelTitle | ||
> | ||
<p>I might be here or I might be gone</p> | ||
</DraggablePanel> | ||
</> | ||
); | ||
} | ||
|
||
render( | ||
<> | ||
<ToggleButtonAndDraggablePanel /> | ||
<DraggablePanel | ||
defaultPosition={defaultPosition} | ||
isOpen | ||
panelTitle="My Extra Ordinary Data" | ||
onDragComplete={() => {}} | ||
onPanelDismiss={() => {}} | ||
showPanelTitle | ||
> | ||
<p>I will be with you forever.</p> | ||
</DraggablePanel> | ||
</> | ||
); | ||
|
||
expect( | ||
screen.getByText("I might be here or I might be gone") | ||
).toBeVisible(); | ||
|
||
const closePanel = screen.getByText("Close My Filters"); | ||
fireEvent.click(closePanel); | ||
|
||
expect( | ||
screen.queryByText("I might be here or I might be gone") | ||
).not.toBeVisible(); | ||
expect(screen.queryByText("I will be with you forever.")).toBeVisible(); | ||
|
||
fireEvent.click(screen.getByText("Toggle Filters Panel")); | ||
expect( | ||
screen.getByText("I might be here or I might be gone") | ||
).toBeVisible(); | ||
}); | ||
test("panels are layered from most-to-least recently dragged", () => { | ||
const panelDefinitionOjects: DraggablePanelProps[] = [ | ||
"Panel 1", | ||
"Panel 2", | ||
"Panel 3", | ||
].map((panelTitle) => { | ||
return { | ||
children: () => <p>Panel Contents</p>, | ||
panelTitle, | ||
showPanelTitle: true, | ||
isOpen: true, | ||
}; | ||
}); | ||
|
||
render(<StackOrderingKeeper draggablePanelProps={panelDefinitionOjects} />); | ||
|
||
const dragMeFirst = screen.getByText("Panel 3"); | ||
const dragMeMiddle = screen.getByText("Panel 2"); | ||
const dragMeLast = screen.getByText("Panel 1"); | ||
|
||
drag(dragMeFirst, { clientX: 50, clientY: 50 }); | ||
drag(dragMeMiddle, { clientX: 60, clientY: 60 }); | ||
drag(dragMeLast, { clientX: 70, clientY: 70 }); | ||
|
||
/** | ||
* Asserting on z-index values makes this test brittle to refactoring. Is there | ||
* another way to programmatically determine stacking order? | ||
*/ | ||
const firstDraggedZIndex = getZIndexValue( | ||
screen.getByTestId(`Panel 3 dragged`) | ||
); | ||
const middleDraggedZIndex = getZIndexValue( | ||
screen.getByTestId(`Panel 2 dragged`) | ||
); | ||
const lastDraggedZIndex = getZIndexValue( | ||
screen.getByTestId(`Panel 1 dragged`) | ||
); | ||
|
||
expect(Number(lastDraggedZIndex)).toBeGreaterThan( | ||
Number(middleDraggedZIndex) | ||
); | ||
expect(Number(middleDraggedZIndex)).toBeGreaterThan( | ||
Number(firstDraggedZIndex) | ||
); | ||
}); | ||
}); | ||
|
||
/** | ||
* So we're pretty limited as regards js-dom and dragging. Here's what I would like to do: | ||
* 1. Simulate dragging events on the draggable element. | ||
* 2. Find the element, getBoundingClientRect for the element | ||
* 3. Assert that the coordinates moved predictably. | ||
* | ||
* Here's the reality: jsdom doesn't do any rendering, so getBoundingClientRect() always | ||
* returns 0,0,0,0. That won't change (even foreseeable long-term). | ||
* You can try to mock the function to emulate the results you'd expect. | ||
* https://github.com/jsdom/jsdom/issues/1590#issuecomment-243228840 | ||
* | ||
* @param element | ||
* @param destinationCoordinates | ||
*/ | ||
function drag( | ||
element: HTMLElement, | ||
destinationCoordinates: { clientX: number; clientY: number } | ||
): void { | ||
fireEvent.mouseDown(element); | ||
fireEvent.mouseMove(element, destinationCoordinates); | ||
fireEvent.mouseUp(element); | ||
} | ||
|
||
type StackOrderingKeeper = { draggablePanelProps: DraggablePanelProps[] }; | ||
function StackOrderingKeeper({ draggablePanelProps }: StackOrderingKeeper) { | ||
const [zIndicies, setZIndicies] = useState<string[]>([]); | ||
|
||
return ( | ||
<div> | ||
{draggablePanelProps.map((props) => { | ||
const zIndex = zIndicies.findIndex( | ||
(panelTitle) => panelTitle === props.panelTitle | ||
); | ||
return ( | ||
<DraggablePanel | ||
isOpen | ||
panelTitle={props.panelTitle} | ||
showPanelTitle | ||
key={props.panelTitle} | ||
onDragStart={() => { | ||
setZIndicies((currentList) => { | ||
return currentList | ||
.filter((panelTitle) => panelTitle !== props.panelTitle) | ||
.concat(props.panelTitle); | ||
}); | ||
}} | ||
styleOverrides={{ | ||
zIndex: zIndex > -1 ? zIndex : "unset", | ||
}} | ||
> | ||
content... | ||
</DraggablePanel> | ||
); | ||
})} | ||
</div> | ||
); | ||
} | ||
|
||
function getZIndexValue(element: HTMLElement) { | ||
return window.getComputedStyle(element).zIndex; | ||
} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
If we stacked two panels exactly atop each other, would something like expecting the top one to be visible and the bottom one not to be visible be viable? Not sure if being "visible" means i can actually read it or it's just on the screen somewhere
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.
That'd be dope!
I think the problem is that
toBeVisible()
is a lot less magical. It comes fromjest-dom
and we're referring to visibility in terms of the DOM. While I suppose you could get thex
andy
coordinates of two elements and see if one is obscuring the other, this "matcher" is much less imaginative:https://github.com/testing-library/jest-dom#tobevisible
I guess what I meant in my comment is that it's possible to have stacking situations that don't involve
z-index
.And the problem with getting the
x
andy
coordinates is that I don't think jsdom supports it: https://github.com/VEuPathDB/coreui/blob/gh-issue-420-add-general-purpose-draggable-panel/src/components/containers/DraggablePanel/DraggablePanel.test.tsx#L159. If you know how to do this, I would be eternally grateful. But, from my understanding, it's because jsdom doesn't implement a full-fledged HMTL rendering engine.