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

💄 GC-1464 Allow users to resize the sidebar in the Content Hub #1296

Merged
merged 34 commits into from
Sep 26, 2023
Merged
Show file tree
Hide file tree
Changes from 29 commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
e5b8600
:construction: PoC for a component that handles resizing with pure CSS
timbryandev Sep 8, 2023
12633d6
:pencil2: use correct export name
timbryandev Sep 8, 2023
e7262ae
:construction: looks like we do need js after all 😞
timbryandev Sep 8, 2023
523fdfc
:fire: remove superfluous stuff from sloppy copy/paste job
timbryandev Sep 8, 2023
1904a4c
:bug: account for the width of the gutter
timbryandev Sep 8, 2023
bf490bc
:pencil2: resizable type
timbryandev Sep 13, 2023
79247db
:recycle: make width stateful
timbryandev Sep 14, 2023
0a38836
:recycle: general cleanup of component post-testing
timbryandev Sep 14, 2023
a589064
:recycle: general cleanup of component post-testing
timbryandev Sep 14, 2023
35faca1
:lipstick: resize bar to have blue line in middle
timbryandev Sep 14, 2023
7b7de8b
:lipstick: add gutter
timbryandev Sep 14, 2023
34a8230
:sparkles: gutter should follow cursor
timbryandev Sep 14, 2023
4d2a42e
:lipstick: remove debugging styles
timbryandev Sep 14, 2023
e6e899d
:recycle: move helpers
timbryandev Sep 14, 2023
42d08cf
:bug: prevent gutter handle from going out-out-of-bounds
timbryandev Sep 14, 2023
761e3ee
:label: correct prop types
timbryandev Sep 15, 2023
2d0d345
:recycle: use ref over state for setting dom element width
timbryandev Sep 15, 2023
92b36eb
:sparkles: gutter offset prop - helps when "position" or "flex" are a…
timbryandev Sep 15, 2023
832a7c5
:coffin: z-index has no effect here
timbryandev Sep 15, 2023
65677b2
:recycle: improve readability of normaliseUnitsToPixelValue
timbryandev Sep 15, 2023
72b83dd
:recycle: better name for `y`
timbryandev Sep 15, 2023
c4b1adb
:recycle: better helper name
timbryandev Sep 15, 2023
3dce17b
:white_check_mark: helper util tests
timbryandev Sep 15, 2023
705cebb
:wrench: include ts in vitest
timbryandev Sep 15, 2023
6b5990c
:lipstick: prevent child elements leaking over the resizer
timbryandev Sep 15, 2023
70aae46
:bug: fix storybook examples
timbryandev Sep 15, 2023
2290f68
:white_check_mark: spy on console.warn
timbryandev Sep 15, 2023
0f5e73c
:lipstick: make examples 100% page width
timbryandev Sep 15, 2023
69183c8
:pencil2: use resizable spelling, containerWidth prop
timbryandev Sep 15, 2023
b1140f0
:zap: keep reference to event handlers for clean event listener removal
timbryandev Sep 26, 2023
6aa07e0
:label: swap jsdoc for better variable names
timbryandev Sep 26, 2023
ed8cb41
:recycle: remove duplicated keepValueWithinRange calculation
timbryandev Sep 26, 2023
3ec581d
:recycle: replace rest prop in favor of prop destructuring
timbryandev Sep 26, 2023
6190595
Merge branch 'main' into feature/GC-1464-resizable-sidebar
timbryandev Sep 26, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 48 additions & 0 deletions lib/.specs/helpers.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
import { describe, expect, it, vi } from "vitest";
import { keepValueWithinRange, toPixels } from "../helpers";

describe("helpers: toPixels", () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

it("should not convert arguments of type number", () => {
expect(toPixels(100)).toBe(100);
});

it("should convert pixels to number", () => {
expect(toPixels(`100px`)).toBe(100);
});

it("should convert percentage to number", () => {
expect(toPixels(`100%`)).toBe(100);
});

it("should convert to a percentage value of the percentageOf arg", () => {
expect(toPixels("50%", 200)).toBe(100);
});

it("should return number and warning for unknown units", () => {
const warn = vi.spyOn(global.console, "warn");

expect(toPixels("123rem")).toBe(123);
expect(warn).toHaveBeenCalledOnce();
expect(warn).toHaveBeenCalledWith(
`Could not interpret a normalised value for: 123rem.\nParsing directly to integer: 123.`
);
});
});

describe("helpers: keepValueWithinRange", () => {
it("should return the value if there are no other arguments", () => {
expect(keepValueWithinRange(50)).toBe(50);
});

it("should return the value if it is within the range", () => {
expect(keepValueWithinRange(50, 33, 77)).toBe(50);
});

it("should return the max if the value is above the max", () => {
expect(keepValueWithinRange(11, 1, 10)).toBe(10);
});

it("should return the min if the value is below the min", () => {
expect(keepValueWithinRange(0, 1, 10)).toBe(1);
});
});
123 changes: 123 additions & 0 deletions lib/Resizable/index.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,123 @@
import type { PropsWithChildren } from "react";
import React, { useCallback, useEffect, useRef } from "react";
import { keepValueWithinRange, toPixels } from "../helpers";

export interface ResizableProps {
defaultWidth?: number | string;
minWidth?: number | string;
maxWidth?: number | string;
useGutterOffset?: boolean;
containerWidth?: number | string;
}

/**
* A wrapper for making a given child element resizable
* @param children The element to be resized
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the reasoning for having types and comment blocks 🤔

They are already out of sync and like in this case shouldn't children be added to the interface but it isn't it's only described in the comment block? 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

For me comment blocks are currently used to explain a complex or smelly piece of implementation, I don't see how the comment blocks add anything other than a maintenance overhead for future refactors. Some ideas to remove the need for the comment blocks...

  • maxWidth to maxResizableWidth
  • minWidth to minResizedWidth
  • defaultWidth to initialWidth
  • add type for children as I am assuming it can't be any react node (e.g. a fragment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks Kyle. I think at the time I was having a bit of trouble trying to remember what props I needed and why I needed them as I was working around new problems and iterating. I'll take them out and refactor the props & interface as advised now the solution is "complete".

Copy link
Contributor Author

@timbryandev timbryandev Sep 26, 2023

Choose a reason for hiding this comment

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

Just had a look back at the code to refresh my memory and we are actually defining the children prop via PropsWithChildren: }: PropsWithChildren<ResizableProps>) {

I still action the rest though. Thanks again for the renaming suggestions as I think the naming was where my initial confusions must have been occuring!

* @param defaultWidth The width the child should initially render to. If min and max widths are provided, then the provided width will be massaged to meet those constraints
* @param maxWidth The maximum width the element can be resized to
* @param minWidth The minimum width the element can be resized to
* @param useGutterOffset Toggle whether we should take the width of the gutter into account for calculating the width of the resized item. This can be handy depending on the CSS being applied to or around the child element
* @param rest Contains params that need converting such as the containerWidth
*/
export function Resizable({
children,
defaultWidth = "50%",
maxWidth,
minWidth,
useGutterOffset = false,
...rest
}: PropsWithChildren<ResizableProps>) {
const containerWidth = toPixels(
rest.containerWidth ?? document.body.offsetWidth
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is containerWidth stored in rest but other properties aren't? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah this doesn't look great. I was lazily looking for a way to dynamically set the default value for containerWidth if it was undefined but I didn't want to do it as a default param in case a falsy value such as 0 gets passed in and then overridden by my default.

I'll clean this up!

);
const resizeRef = useRef<HTMLDivElement>(null);
const resizeWrapperRef = useRef<HTMLDivElement>(null);
const handleRef = useRef<HTMLSpanElement>(null);
const gutterSize = useGutterOffset ? 16 : 0;
const min = toPixels(minWidth ?? 0, containerWidth);
const max = toPixels(maxWidth ?? "100%", containerWidth);

const [state, setState] = React.useState({
startX: 0,
startWidth: 0,
});

const getWidth = () => toPixels(resizeWrapperRef.current?.style.width || 0);

const setWidth = (value: number) => {
if (resizeWrapperRef.current === null) return;

resizeWrapperRef.current.style.width = `${
keepValueWithinRange(value, min, max) - gutterSize
}px`;
};

const doDrag = (evt: MouseEvent) => {
const newWidth = state.startWidth + evt.clientX - state.startX;
setWidth(newWidth);
};

const stopDrag = () => {
document.removeEventListener("mousemove", doDrag, false);
document.removeEventListener("mouseup", stopDrag, false);
Copy link
Contributor

Choose a reason for hiding this comment

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

These events will never be removed.

This is because on re-render stopDrag and doDrag will have a different signature and removeEventListener won't work.

Talk to @micmcgrorty about this, we had a similar issue with editor server events recently.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interesting point! I'll ask Michael about it :)

};

const initDrag = useCallback(
(evt: React.DragEvent<HTMLDivElement>) => {
const { clientX } = evt;

const newState = {
...state,
startX: clientX,
startWidth: getWidth(),
};

setState(newState);

document.addEventListener("mousemove", doDrag, false);
document.addEventListener("mouseup", stopDrag, false);
},
[setState]
);

const setGutterHandlePosition = (evt: React.MouseEvent<HTMLSpanElement>) => {
const handle = handleRef.current;
if (handle === null) return;

const rect = evt.currentTarget.getBoundingClientRect();
const gutter = handle.parentElement as HTMLSpanElement;
const handleOffset = handle.offsetHeight / 2;

const yPosition = keepValueWithinRange(
evt.clientY - rect.top,
handleOffset,
gutter.offsetHeight - handleOffset
);

handle.style.top = `${yPosition - handleOffset}px`;
};

useEffect(() => {
const newWidth = keepValueWithinRange(toPixels(defaultWidth), min, max);
setWidth(newWidth);

// remember to remove global listeners on dismount
return () => stopDrag();
}, []);

return (
<div ref={resizeRef} className="resizable">
<div ref={resizeWrapperRef} className="resizable__wrapper">
{children}
<span
role="none"
className="resizable__gutter"
onMouseDown={initDrag}
onMouseMove={setGutterHandlePosition}
>
<span ref={handleRef} className="resizable__gutter-handle" />
</span>
</div>
</div>
);
}
55 changes: 55 additions & 0 deletions lib/Resizable/styles.scss
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
$gutterSize: 16px;
$gutterColour: #006aff;

.resizable {
position: relative;
display: flex;

&__wrapper {
position: relative;
display: flex;
align-items: stretch;
}

&__wrapper > *:first-child {
width: 100% !important;
overflow-x: hidden;
}
}

.resizable__gutter {
position: absolute;
user-select: none;
height: 100%;
width: $gutterSize;
top: 0;
right: -(calc($gutterSize / 2));
cursor: ew-resize;

&::before {
content: "";
width: 2px;
margin: auto;
background-color: $gutterColour;
display: block;
height: 100%;
}
}

.resizable__gutter-handle {
background-color: $gutterColour;
border-radius: 4px;
height: 30px;
margin: auto;
width: 8px;
display: flex;
position: absolute;
transform: translateX(-50%);
left: 50%;
opacity: 0;
transition: opacity 0.1s;

.resizable__gutter:hover & {
opacity: 1;
}
}
51 changes: 42 additions & 9 deletions lib/helpers.ts
Original file line number Diff line number Diff line change
@@ -1,16 +1,49 @@
export const pluralisePerson = (count: any) => count === 1 ? `${count} person` : `${count} people`;
export const pluralisePerson = (count: any) =>
count === 1 ? `${count} person` : `${count} people`;

export const pluraliseHas = (count: any) => count === 1 ? 'has' : 'have';
export const pluraliseHas = (count: any) => (count === 1 ? "has" : "have");

export const filterUsers = (users: any, term: any, searchByEmail = false) => {
const safeTerm = term.toLowerCase();
return users.filter(
(user: any) => user.name
.toLowerCase()
.split(' ')
.filter((subStr: any) => subStr.lastIndexOf(safeTerm, 0) === 0).length > 0 ||
user.name.toLowerCase().lastIndexOf(safeTerm, 0) === 0 ||
user.display.toLowerCase().lastIndexOf(safeTerm, 0) === 0 ||
(searchByEmail && user.email.toLowerCase().lastIndexOf(safeTerm, 0) === 0)
(user: any) =>
user.name
.toLowerCase()
.split(" ")
.filter((subStr: any) => subStr.lastIndexOf(safeTerm, 0) === 0).length >
0 ||
user.name.toLowerCase().lastIndexOf(safeTerm, 0) === 0 ||
user.display.toLowerCase().lastIndexOf(safeTerm, 0) === 0 ||
(searchByEmail && user.email.toLowerCase().lastIndexOf(safeTerm, 0) === 0)
);
};

timbryandev marked this conversation as resolved.
Show resolved Hide resolved
export const toPixels = (value: string | number, percentageOf = 100) => {
if (typeof value === "number") return value;

const integer = parseInt(value, 10);

if (value.endsWith("px")) {
return integer;
}

if (value.endsWith("%")) {
return (percentageOf / 100) * integer;
}

console.warn(
`Could not interpret a normalised value for: ${value}.\nParsing directly to integer: ${integer}.`
);
return integer;
};

export const keepValueWithinRange = (
start: number,
min?: number,
max?: number
) => {
let value = start;
if (typeof max === "number") value = Math.min(value, max);
if (typeof min === "number") value = Math.max(value, min);
return value;
};
1 change: 1 addition & 0 deletions lib/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ export { NotificationInlineWarning } from "./Notification/inline/NotificationInl
export { Dropdown } from "./Dropdown";
export { DropdownMenu } from "./DropdownMenu";
export { ProgressButton } from "./ProgressButton";
export { Resizable } from "./Resizable";
export { Progress } from "./Progress";
export { SearchInput } from "./SearchInput";
export { Modal } from "./Modal";
Expand Down
Loading
Loading