-
Notifications
You must be signed in to change notification settings - Fork 140
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
Reusable resizable flex HOC #1488
Conversation
@pwjablonski Would love to get your thoughts on this! All feedback very welcome, but in particular I’m interested in whether this seems straightforward enough from an API consumer standpoint (e.g. would it be intuitive to use it to add flex resizing to more parts of the UI) |
94618ad
to
0c29a71
Compare
src/components/EditorContainer.jsx
Outdated
}; | ||
|
||
export default EditorContainer; | ||
export default React.forwardRef(EditorContainer); |
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 a much nicer way to do what we need to do here, but it doesn’t currently work with connected components, so I wasn’t able to do this refactor for <Workspace>
x, | ||
}); | ||
} | ||
|
||
_renderEnvironment() { |
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 change will make it substantially easier to decompose <Workspace>
into several smaller presentational components, but that should come in a separate PR.
f61b9fa
to
09b0c0e
Compare
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.
Hey @outoftime sorry for the delay here! This is awesome! From a consumer standpoint, this looks pretty straightforward to implement. That being said, I can't say that I 100% understand what is going on underneath the hood (in src/util/resizableFlex.js) but would be comfortable to implement on another set of resizable components. Also there are a couple of things that I need clarification on, mostly for my own edification. See comments.
Also had a couple of errors logged to the console.
forwardRef render functions do not support propTypes or defaultProps
isExperimental
Failed prop type intopBar
andhamburger
projectKey
Failed prop type ininstructions
@@ -45,4 +40,4 @@ function mapDispatchToProps(dispatch) { | |||
export default connect( | |||
mapStateToProps, | |||
mapDispatchToProps, | |||
)(Workspace); | |||
)(resizableFlex(2)(Workspace)); |
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 clear - allows for 2 resizable containers in the workspace component.
I don't fully understand how connect works and how/why passing both resizableFlex(2)(Workspace) here works.
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.
@pwjablonski good question. The basic situation here is that resizableFlex
is a function that returns a function that returns a component. So, calling resizableFlex(2)
means “make me a function that takes a component, and decorates it with a controller for a size 2 resizable flex container, returning the decorated component”. Then calling that function with the Workspace
component as an argument gives us the actual decorated component.
Does that clarify?
Also occurs to me that this PR really needs some tests. In particular I am wondering if the inner workings of the resizableFlex
module may be a bit easier to understand with the aid of some unit tests. Of course I should also add tests for the new reducer.
09b0c0e
to
371d4e7
Compare
0d3d8d9
to
3533c14
Compare
OK @pwjablonski updated with some tests and some mildly improved code organization. Take another look? Would love to know if the implementation is any clearer now that there are some unit tests, and do let me know if there are specific areas that don’t make sense! |
Adds a higher-order component that injects drag-to-resize flex functionality into any component with a draggable flex layout. All calculations and state are managed by the HOC, reducing a presentational component’s responsibilities to forwarding refs, using the flex values given, and delegating drag events back to the handler prop. Unlike the previous implementation, the logic here follows a simple rule that dragging a divider will only affect the size of the regions adjacent to that divider. While there is no obvious right or wrong answer here, the new behavior is consistent with other drag-to-resize implementations I’ve played with, and makes it particularly easy to support an arbitrary number of flex regions. Anyway, the fundamental structure of this code could accommodate different logic, so it wouldn’t be difficult to refine in the future. The implementation here assumes that the final size of flex regions will be primarily determined by the `flex-grow` property: the initial main size represents the _smallest_ that the region should be, and then the regions grow to fill available space. So, dragging to resize simply calculates what the ratio of `flex-grow` between the two divider-adjacent regions should be, and updates them accordingly. If a drag would cause one of the regions to be smaller than its initial main size, it is ignored.
Applies new `resizableFlex` higher-order component to the `<Workspace>` component to control resizing of the flex regions containing the editors column and output respectively. Fully removes the tightly-coupled flex resizing support from Redux and React infrastructure. Differences between calculating flex for row and column flex directions are encapsulated in a pair of adapters. Flex direction-specific property access now in adapter Fix adapter usage Use generic HOC for drag-to-resize editors column and preview Only works in one direction because of pointer capture Restore start/stop drag Fix lint Remove explicit Redux-level support for drag-to-resize Removes support for resizing the center vertical divider, now that this functionality has been migrated to the resizeable flex module. One dangling rowflex reference
Ensures we don’t propagate prop updates from the resizable flex HOC unless something relevant has actually changed. The main functions in the `resizableFlex` module are independent of the current state, so we should just define them once and then merge them into the final props returned by the component. The props-returning function should return the same instance as previously if the props themselves are unchanged. To do this, we want to memoize the function so that new props are only returned if: * The flex list (received from Redux) has changed * The `ownProps` (passed directly to the component) have changed In the latter case, we can’t rely on `ownProps` to be strictly equal, however; instead, we need to create a custom memoized function that does a shallow value check (the same kind of check used for pure `react-redux` containers)
3533c14
to
a589c7a
Compare
Adds a higher-order component that injects drag-to-resize flex functionality into any component with a draggable flex layout. All calculations and state are managed by the HOC, reducing a presentational component’s responsibilities to forwarding refs, using the flex values given, and delegating drag events back to the handler prop.
Unlike the previous implementation, the logic here follows a simple rule that dragging a divider will only affect the size of the regions adjacent to that divider. While there is no obvious right or wrong answer here, the new behavior is consistent with other drag-to-resize implementations I’ve played with, and makes it particularly easy to support an arbitrary number of flex regions. Anyway, the fundamental structure of this code could accommodate different logic, so it wouldn’t be difficult to refine in the future.
The initial flex layout of the regions is taken as a given, and should be set up using normal CSS rules in the stylesheet. This means that the resizing module makes no assumptions about what the initial size ratio between the different regions is.
Actual resizing is done exclusively by changing the
flex-grow
property to match the size ratio implied by the drag action, so the initial layout of the regions should useflex-grow
as the primary means of sizing. The module explicitly prevents regions from being resized smaller than their “initial main size” (the size they would be without any flex grow or shrink), so a large flex basis with flex shrink won’t work.This PR does not add any new flex resizing, primarily because the two areas where it would be most useful—the instructions bar and the console—don’t currently have the right DOM layout to afford it. Changing the DOM layout on top of everything that’s already in here would yield a substantially oversized PR. Anyway, the direct motivation for this change is a code quality improvement (encapsulating the flex resizing concern in a smaller number of code locations), which is accomplished by the changes herein.
To do