💄 GC-1464 Allow users to resize the sidebar in the Content Hub #1296
Conversation
timbryandev
changed the title
timbryandev
requested review from
webknit,
lewishankinson,
AmeeMorris and
micmcgrorty
timbryandev
changed the title
lib/Resizable/index.tsx
Outdated
|
|
||
| /** | ||
| * A wrapper for making a given child element resizable | ||
| * @param children The element to be resized |
There was a problem hiding this comment.
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? 🤔
There was a problem hiding this comment.
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...
maxWidthtomaxResizableWidthminWidthtominResizedWidthdefaultWidthtoinitialWidth- add type for
childrenas I am assuming it can't be any react node (e.g. a fragment)
There was a problem hiding this comment.
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".
There was a problem hiding this comment.
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!
lib/Resizable/index.tsx
Outdated
| ...rest | ||
| }: PropsWithChildren<ResizableProps>) { | ||
| const containerWidth = toPixels( | ||
| rest.containerWidth ?? document.body.offsetWidth |
There was a problem hiding this comment.
Why is containerWidth stored in rest but other properties aren't? 🤔
There was a problem hiding this comment.
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!
lib/Resizable/index.tsx
Outdated
|
|
||
| const stopDrag = () => { | ||
| document.removeEventListener("mousemove", doDrag, false); | ||
| document.removeEventListener("mouseup", stopDrag, false); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Interesting point! I'll ask Michael about it :)
| import { describe, expect, it, vi } from "vitest"; | ||
| import { keepValueWithinRange, toPixels } from "../helpers"; | ||
|
|
||
| describe("helpers: toPixels", () => { |
💬 Description
A new component designed to wrap existing elements and make them resizable. In this specific instance we're keeping the sidebar menu in mind.
Within the vite config, It looks like most of the changes are formatting related. However, there is an actual change on line 11 were I've added tsx (
(x)?) to the include list:include: ["**/.specs/**/*.+(spec).ts(x)?"],Recording:
https://www.loom.com/share/9bd52f0c9d9248258b4f7eaa57b7efe7
🔗 Links