Add resizing for Design Preview and Testing Results panes

[simplywondrous]

Context

This PR closes #2583

Changes

• Introduced renderPaneOne and renderPaneTwo in PageLayout
  • This rendered the wrapper grid div that wrapped the middle and right panes unnecessary, so that was removed
  • Consolidated CSS styling - removing styles now unused after these changes
  • For cases where panes aren't desired, renderPageBody is still available (such as for WrapperHome)
• Added ability to resize middle / right panes to Design and Test tabs

GIF

[simplywondrous]

There are three ways I'm thinking of for cleaning this up -

1. Forwarding the refs with the wrapper (that wraps the two panes) with forwardRef instead of creating classes. Doesn't seem like forwardRef is used anywhere, and React has a note to component library maintainers about introducing it. This is an app and not a component library I guess, but asking to be sure!

2. Keep the grid wrapper, slap the refs directly onto the dom elements themselves, and modify the resize logic to be

      const requestPane =
        this._requestPane instanceof HTMLElement
          ? this._requestPane
          : ReactDOM.findDOMNode(this._requestPane);
      const responsePane =
        this._responsePane instanceof HTMLElement
          ? this._responsePane
          : ReactDOM.findDOMNode(this._responsePane);

so that we get correct offsetWidth values from request and response panes, regardless of if the ref is an element or component
(I am currently stuck with this one. The ref element is correct, but the offsetWidth value isn't updated as I drag - if you have any ideas for what might be the issue?)

3. Change the wrapper to a flexbox, which handles resizing easier than a grid layout

For question / option #2 - here's what renderPageBody looks like:

renderPageBody() {
  ...
  return (
    <div className="unit-tests layout-body--sidebar">
      <section ref={handleSetRequestPaneRef} className="unit-tests__tests">...</section>
      <div className="drag drag--pane-test">...</div>
      <section className="unit-tests__results results-pane" ref={handleSetResponsePaneRef}>
         {this.renderResults()}
      </section>
    </div>
  );
}

[develohpanda]

All in all, a nice approach you've followed 👏 Code looks good!

Note: if none of the following options are viable in the short term, the current changes (and those anticipated for WrapperDesign) are still great! I have some really minor notes but we can address those once the PR is ready with the desired direction.

To answer your questions

Forwarding the refs with the wrapper (that wraps the two panes) with forwardRef instead of creating classes.

I'm not too concerned about the warnings for library maintainers with this particular case as we are the only consumers of the components. How do you envision forwardRef being set up? To my understanding, forwardRef works with a single ref, but we have multiple to pass around (sidebar, request, response panes).

Alternatively the new class components can also be simple(r) function components. New components might be unnecessary altogether too, if you get refs on dom elements working.

slap the refs directly onto the dom elements themselves

I'm not sure why this wouldn't be working... could it be that you need to use this._requestPane.current when accessing a ref in the absence of findDOMNode?

If you can't get it working for this PR, it can be scoped to a standalone PR given it might need some additional work, but it would be good to remove findDOMNode (unless absolutely necessary) as it is now discouraged. Looks like the pane dragging logic was written in 2016, so is probably well outdated with current React practices. 🤣

Change the wrapper to a flexbox, which handles resizing easier than a grid layout

While this would be pretty awesome, my gut feel is that it might be a lot of work. CSS Grid is quite central to the layout and it's variations. Flexbox would definitely be a step forwards but might be too much to tackle in this PR?

Some additional observations

I was looking at how the drag pane was set up for the sidebar, because it seems to be working across all views (design, debug, test) in the current state. It looks like PageLayout has renderPageSidebar and renderPageBody as props, and sets up the sidebar dragging logic in there.

I wonder if it's worth investigating whether renderPageBody can be updated to renderPaneOne and renderPaneTwo, and any className props, and the dragging logic for the entire layout all be set up in one common location.

This doesn't really change the approach, just prevents the common dragging logic from being scattered and duplicated throughout. It might be slightly more work than just duplicating but feels a little more maintainable. What do you think?

[simplywondrous]

It's only a guess, but after thinking about it some more, maybe if the ref is a dom element since the element itself isn't changing on drag no update is triggered, whereas when the ref is on a class component (that then finds a dom element) when the pane gets resized something is updated and the changes propagate down to the component, which then updates?

Looking it up, seems the best way to forward multiple refs is to make the ref an object with refs. That being said, if we had PaneOne and PaneTwo then we wouldn't need to forward multiple, so that seems to be the way to go. Class components can be assigned refs because an instance exists whereas function components require forwardRef, so I'll just make them class components.

It'd make the most sense if the dragging logic were all in one place like PageLayout. I haven't looked at how the functions in app.js (including the drag logic) make it into PageLayout, but I think for this PR I'll try to separate renderPageBody into renderPaneOne and renderPaneTwo and consolidate the styling, along with adding resizing between the two panes. Then removing findDOMNode and seeing if the drag logic can be moved to PageLayout can be follow-up issues.

[develohpanda]

My understanding of ReactDOM.findDOMNode is that if invoked with a ref attached to a component, it returns the first dom element rendered by the component, and so should be the same as just putting a ref on the dom element itself (as recommended). Neither the element nor the component is updated (as far as I can tell) because paneWidth and paneHeight are consumed higher up the tree in PageLayout. Needs some more research but I wonder if findDOMNode is loading the latest dom before finding the node, which would have been updated with the row/column sizing applied in PageLayout. What a pickle!

for this PR I'll try to separate renderPageBody into renderPaneOne and renderPaneTwo and consolidate the styling, along with adding resizing between the two panes.

This sounds good 👍 I can't think of any (great) reasons not to split renderPageBody - in the case where only one of the panes is used (preview hidden in the design tab), one of the render props can just be optional, and drag handlers can be applied as required within PageLayout.

Then removing findDOMNode and seeing if the drag logic can be moved to PageLayout can be follow-up issues.

I agree, it seems non-trivial, but also non-critical so it can be tackled as a standalone PR. Another thing to be tackled separately is persisting the unique sidebar/pane widths per workspace and per tab. Previously we only had one tab (Debug) so it wasn't necessary.

[simplywondrous]

Ohhh I see - so if findDOMNode is loading the latest dom and then finding the node, it'd get the updated offset, whereas when we're simply looking at the current ref element's offset, that doesn't get updated so stays the same.

Right now I'm working on hiding PaneTwo when hiding the preview in the design tab, and have a question.
Currently, render in PageLayout includes (with some code omitted)

return {renderPaneTwo && <div> ... renderPaneTwo()</div>}

and in WrapperDesign, renderPaneTwo={this.renderPreview} is passed to PageLayout, with

renderPreview = () => { return !previewHidden && <div /> }

Since we're passing it a function, renderPaneTwo is always true even when renderPaneTwo() returns false. To fix this, my first thought is to call renderPreview in WrapperDesign and pass the returned value to renderPaneTwo:

const preview = renderPreview()
<PageLayout renderPaneTwo={preview} />

This would prevent dynamically generating the content of PaneTwo in PageLayout, though. Then preview from the example above would be statically passed in. I'm pretty sure this would cause edge case issues of some sort, but am not quite skilled enough to give examples, so could be I have concepts mixed up 😔. Maybe if renderPreview called a handler that relied on variables set elsewhere which later changed?

If there are no issues, would it be a good idea to change just renderPaneTwo to take a JSX element instead of a function, while keeping the rest, such as renderPaneOne, the same?

Second thought is, in PageLayout I could do

const preview = renderPaneTwo()
return {preview && <div />}

and I think then the content would still be dynamically generated upon component render. Would this be okay or is there a better way to go about hiding the second pane?

Update: Even after implementing the last suggestion, I'm still running into an issue where PaneOne's grid space is calculated by the parent Wrapper and updated when PaneTwo is dragged, but not when PaneTwo doesn't render, similar to what happened when I didn't use findDOMNode. I'll have to think on it more, but to get hiding the preview (in WrapperDesign) to trigger a style change in its parent component (PageLayout) would require the parent passing down a show/hide handler of some sort to PaneTwo to then call, right?

[develohpanda]

There are a few different options I can think of, and no one is better than the other at first sight. The options you've given are both good too.

I'll pull this code later today and see what's up, AFK for the next few hours! 😊

[simplywondrous]

I very much appreciate your responsiveness, but it says your status is "on vacation" so please enjoy it! This is certainly not something that can't wait a few days!

[develohpanda]

I very much appreciate your responsiveness, but it says your status is "on vacation" so please enjoy it! This is certainly not something that can't wait a few days

I picked a good week to take off, middle of the busiest open-source month 😅 just making sure the wheels keep turning, but I'll dip away for the last few days; thanks for noting it!

I noticed WrapperHome is yet another variation of the grid which will need to be considered here. This is the documents home page when you hit the "Documents" link on the top left. Note: with this PR in the current state, the homepage doesn't show anything, because it's still using the old renderPageBody prop.

If there are no issues, would it be a good idea to change just renderPaneTwo to take a JSX element instead of a function, while keeping the rest, such as renderPaneOne, the same?

Because the render functions don't actually have any arguments, I don't think there's a specific case for them to be functions over just React.Element or React.Node. Might be worth doing some testing here to validate this, but if you change them it should behave like for like. I'd suggest paneOne and paneTwo be React.Node or React.Element (since they are directly impacted by this PR), but the other ones remain as functions as-is for now.

FYI the Header component has React.Node props (used in all Wrapper components as well):

insomnia/packages/insomnia-components/components/header.js

Lines 5 to 10 in 4fc8b21

	type Props = {|
	className?: string,
	gridLeft?: React.Node,
	gridCenter?: React.Node,
	gridRight?: React.Node,
	|};

my first thought is to call renderPreview in WrapperDesign and pass the returned value to renderPaneTwo

Lastly, as far as I know, this is alright. It's a fairly common pattern. React components are, after-all, standard JS objects, and they can be passed around as props just fine.

One gotcha

const preview = renderPreview()
<PageLayout renderPaneTwo={preview} />

Assuming the above snippet runs in each render of WrapperDesign, depending on the implementation preview would be a brand new JS object, and it will cause PageLayout to update because props will be shown as changed. Is this the concept you are thinking about, regarding the edge case?

It does but also doesn't matter (here) because there are other props using inline functions () => {} as props, which causes the same behavior (brand new function each render), causing the PageLayout to update.

If you get rid of the render functions, does the drag resizing work as expected?

[simplywondrous]

Pushed a couple more changes and updated the PR body! Still have a couple todos, but please let me know if the logic looks okay, and if I'm missing something not in the todos!

[develohpanda]

Consolidated CSS styling, mostly removing styles previously unused or now unused after these changes

One note about styles, in favor of keeping the scope of this PR small. If styles were previously unused, move to a separate PR. If styles are now unused after these changes, keep in this PR. 😄

Looks to be on the right path! I cloned locally and ran it again, and am quite impressed with what you have achieved. 👏

I'm not so familiar with the CSS classes and where they are used upfront, but as long as the end result is correct, I'm not too concerned about the changes made there.

Also, it is possible to create custom themes via Insomnia plugins. It would be worth comparing the live application with your PR, and researching any of the removed classes, to ensure theme support hasn't changed.

For example, the Core Default theme in Insomnia is fairly broken for Designer, but it would be good to understand why there are differences between this PR and 2020.4.1. It will be something to do with CSS classes and hierarchy.

Screenshots

[simplywondrous]

Aww shucks, I even tried out a bunch of different themes but I guess not the first one! It was missing two properties - it should be good now!

I'll keep the CSS changes as is in this PR and open another new PR with just the previously unused styles, if that one can be merged in first?

And thanks for the compliment, it means a lot!

[develohpanda]

QA:

• horizontal resizing panes work on all tabs
• vertical resizing panes work on all tabs
• automatic/forced vertical layout works 🤯
• designer homepage correctly wraps and scrolls with multiple documents
• view -> toggle sidebar (to hide sidebar) works on all tabs
• after navigating around, I don't see any additional errors or warnings in the console window
• can send requests (E2E tests)
• double check the on-boarding flow (E2E tests do cover this)

Nicely done! I think there ended up being some overlap with #2713, because this PR handles vertical layout automatically, but this PR would be my preferred direction. I did find another bug (#2787) that might already be fixed by #2713, though.

LGTM, thank you for working through the various iterations, and putting your time into it! I'm looking forward to merge it in. 💯

[develohpanda]

Apologies for the merge conflicts! There was some refactoring in #2799, and looking at that PR might help. The changes in the conflicting files were mostly copy-paste so it should not be too tricky. 🤞 Let me know if you need any help resolving them.

[simplywondrous]

Merged and updated :)

[develohpanda]

Merge looks good! Any future conflicts should only be prop changes to GrpcRequestPane and GrpcResponsePane. Unfortunately there is another piece of work in progress that touches this code, but all of the foundational work is in place and I don't anticipate any more major conflicts.

[DMarby]

There's still some dead code in the render() function of WrapperDesign, but as long as that gets removed this looks good to go: #2712 (comment)

Thanks for implementing this @simplywondrous!

[develohpanda]

Just keeping an eye on merge conflict resolution and noticed some props need to be sent through 👀 should be a quick one

gRPC work is 90% complete so there will not be any more conflicts 🤞

[develohpanda]

🎉

[DMarby]

[simplywondrous]

Is this PR waiting for another review? Otherwise could someone merge it in?

[develohpanda]

We've held off on merging OS PRs for the last week or so, pushing for a big release. Now that 2020.5 is on the horizon, we'll start merging OS PRs again. No more reviews needed, this PR is good to go. 😄

[roqueemir]

@