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

Implementation to allow right panel's resizing and to go into full-screen mode #24

Merged
merged 13 commits into from
Nov 10, 2015

Conversation

thachmai
Copy link
Contributor

This is my first implementation for issue #23. It groups the Outline + Inspector into a component called "RightPanel" and adds the capability to close/open the panel by clicking on the vertical bar.

I'm no stranger to web development, but this is literally my first commit with React, Coffeescript and Stylus. It'd be great if you can let me know what you think about it and whether I should pursue this route further.

If the commit is OK with you, I can pursue it further to cleanup/improve the implementation. Some ideas:

  • Implement a cleaner mechanism to store the layout state.
  • Re-implement a better way to propagate the "resize" event to Canvas element (I'd likely need help with this since I'm really new to React).
  • Improve the styling of the vertical resize bar. Maybe add a visual indicator that the bar is click/draggable.
  • Add the capability to resize the RightPanel instead of just closing/opening.
  • Extend the concept to the CreatePanel to provide a full screen canvas.

@electronicwhisper
Copy link
Collaborator

This is a great start!

Your hack to trigger the resize event is fine. I'd leave it like that for now with a comment that it's there to get the main canvas to resize.

I think the right side bar should resize when you drag it. And then to go "fullscreen", there should be a menubar item. (The menubar will eventually be organized in a hierarchy rather than flat. This fullscreen option will be in the View menu, along with zoom in/zoom out, etc.) When you go into fullscreen mode, only the canvas will be shown (create bar, right panel, and menubar will be hidden). There will be a little button in the upper-right corner that toggles you back out of fullscreen mode. What do you think?

Thanks for the work so far. 👍

@thachmai
Copy link
Contributor Author

Thanks for the encouragement :)

Your suggestions make good sense to me. The button placement for full-screen and edit mode troubles me a bit though. We'd end up having the "full screen" button in the menu, and the button to return to "edit" mode floating in the top right corner...

Microsoft OneNote application has a similar functionality, but they put the button in a static location at the top right corner. Push it once => full screen, push it again => edit. This could make make the interface more consistent.

Did I manage to make sense with the explication? Let me know if you want a screenshot.

@electronicwhisper
Copy link
Collaborator

Yes I see what you're saying. Sounds good.

},
R.div {
className: "RightResize"
ref: "resize"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of giving this a ref in order to make it respond to mousedown (in line 22-23), you can just give this a onMouseDown prop. That is, change the line to:

onMouseDown: @_onResizeMouseDown

This will call @_onResizeMouseDown with the expected mouseDownEvent.

@thachmai
Copy link
Contributor Author

OK I pushed everything to my branch.

Thanks for the tip on the "onMouseDown" event. I refactored it accordingly.

As you can see, in the end I put the full-screen toggle button in the menubar. I got lots of "inspiration" from the Mac OS fullscreen button ;)

Please take a look at the whole thing and let me know what you think. As always, recommendations/suggestions are greatly appreciated.

@thachmai thachmai changed the title A draft implementation to close/re-open the right panel to increase the canvas size Implementation to allow right panel's resizing and to go into full-screen mode Oct 25, 2015
@electronicwhisper
Copy link
Collaborator

Looking good. Here are some ideas:

It would be nice if there was less layout logic within each of the "section" views (CreatePanel, Canvas, RightPanel, Menubar). If we were to use flexbox to lay out these sections, I think we could make RightPanel resize by adjusting its width, and then have Canvas just take up the remaining space using flexbox, without having to have any layout logic within View/Canvas. Perhaps similarly with Menubar. So the idea is instead of doing absolute positioning as we're doing now, we use flexbox and show/hide/change the width of things and have canvas take up all the remaining space. (I'm not an expert on flexbox but I think this is possible and would make the code cleaner.)

Similarly, instead of putting a Fullscreen class on Canvas, CreatePanel, Menubar, and RightPanel, we could just put a Fullscreen class on Editor and use CSS appropriately. Or, I would even consider making Editor not render CreatePanel, Menubar, and RightPanel at all if it's in fullscreen mode. That is, not just display: none them, but not render them at all. (I think this would actually make fullscreen mode actually perform much faster than non-fullscreen mode.)

I should let you know: my plan for making diagrams embeddable on web pages is essentially fullscreen mode (no sidebars) where you can only drag shapes that are controllers (the ones that highlight red). So the fullscreen mode that you've put together may evolve into this "preview" mode, where essentially you're seeing/manipulating your diagram as it would appear embedded on an external web page. So thank you for putting this together so far: it is bringing the embedding feature closer, which is very important.

For the right sidebar, I would like to remove the extra grey space, the 10 pixel border between the sidebar and the canvas. Perhaps a nice way to do this would just be to make that grey area transparent and overlayed on the canvas, so that the canvas shows through. And then make that transparent area have a cursor: ew-resize, so that as you play with Apparatus you can discover that the right sidebar is resizeable. I think this would keep things visually tight while still implementing the desired functionality.

@thachmai
Copy link
Contributor Author

thachmai commented Nov 1, 2015

I'm starting to experiment with the flex layout and I think it's workable. It is a great idea and would help to reduce the complexity of the layout system in general. I'll hack at it for a bit more to see if the whole thing can work.

Are you comfortable with this one pull-request that changes the layout system + implement the full-screen mode at the same time? I can open a new pull-request for the layout refactoring if you want.

@electronicwhisper
Copy link
Collaborator

The one pull request is fine. Whatever's easiest for you.

@thachmai
Copy link
Contributor Author

thachmai commented Nov 8, 2015

The new implementation with display: flex is up. Took a bit longer than I expected.

Compared to commit 7c1484f, here are the major changes:

  • display: flex for all removable components in fullscreen mode.
  • The components simply do not render in fullscreen mode.
  • Restored the old style for right panel. The resize bar is an invisible 10px div with cursor: ew-resize for visual aide.

This implementation does result in noticeable speed-up when manipulating groups with spread. Canvas zooming speed might be improved as well but it's much less noticeable sur my laptop.

This change does impact a lot of the layout css. Please take a careful look and let me know what you think.

"LayoutMode": true
"FullScreen": layout.fullScreen
}
ref: "LayoutMode"
Copy link
Collaborator

Choose a reason for hiding this comment

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

don't need this ref

@electronicwhisper
Copy link
Collaborator

Getting really close!

There's a bug with the create panel thumbnail canvases being resized to the wrong size. Not exactly sure what it's about, but I assume something with the flexbox layout.

image

The icon svg images are referenced now as absolute urls, e.g. url(/icons/fullscreen.svg). I haven't been running the editor as the "root" url, so for now let's just change these to url(../icons/fullscreen.svg). I had started putting together an infrastructure for "font icons", but I'm not convinced that this is better than just referencing svg images directly. The advantage of the font icons is it's easy to change their color/opacity, so you can do hover effects in css. Feel free to try out the font icon thing or just reference the svg images as you do; we'll do a pass on visual polish of icons at some point in the future.

Because you use relative resizing (xDelta) to implement the right sidebar resize and because there is a min/max size, if you drag past the bounds and then drag back, the sidebar no longer is "synced" with the mouse. I would suggest this variation on the implementation:

  1. Get the current size of the right panel on mouse down, and then store the difference between mouse x and this original size as the "offset".
  2. On mouse move, set the absolute size of the right panel based on the current mouse x and the offset from step 1.

And that's it! These are the only issues I've found. Thanks again, this is great work!

@thachmai
Copy link
Contributor Author

I fixed the styling issue for the x button and the resizing behavior.

The fullscreen and edit buttons are now in the icon font as well. I struggled a bit to find the compilation method for fontcustom. In your readme, you pointed to the npm module fontcustom which didn't seem to do a whole lot. I ended up installing https://github.com/FontCustom/fontcustom/ with homebrew and got it compiled. Is that the way you use it as well? Is it possible to compile with a node module instead?

@electronicwhisper
Copy link
Collaborator

I think this looks done! I will merge it.

The readme links to fontcustom.com which apparently is down. The link you found is what I was using. I'll update the readme to point at that instead.

electronicwhisper added a commit that referenced this pull request Nov 10, 2015
Implementation to allow right panel's resizing and to go into full-screen mode
@electronicwhisper electronicwhisper merged commit 592a824 into cdglabs:master Nov 10, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants