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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Docks #13977

Merged
merged 40 commits into from Mar 30, 2017

Conversation

Projects
None yet
6 participants
@matthewwithanm
Member

matthewwithanm commented Mar 10, 2017

Hey all! I wanted to get this up to get some eyes on it and discuss some stuff. I'm trying to keep all the commits very focused to make things easy to review.

Stuff to Do

  • Add Dock component
  • Update atom/tabs to decorate docks
  • Serialize docks with workspace
  • Update workspace.open() to be location-aware
  • Remember (and re-use) last known location when opening
  • Add WorkspaceCenter class and workspace.getCenter() method
  • Update workspace pane inspection methods (that don't have to do with active panes) to reflect all locations
  • Don't create a Panel for docks
  • Update event listening strategy for showing toggle buttons
  • Swap LocalStorage for StateStore
  • Add tab check to drag handler
  • Add new workspace.toggle() method
  • Fix failing tests 馃槥
  • Add tests for new functionality/APIs 馃槥
  • Update Nuclide to new APIs, see if anything breaks
  • Fix issue: "core:save" only saves active item in center (#14027)
  • [atom/flight-manual] Update docs
  • [atom/tabs] Fix issue: Dragging overlay not showing up when over a "peaked" dock. (z-index?) (atom/tabs#425)
  • [atom/tabs] Add tests to make sure docks are decorated (atom/tabs#429)

Questions/Stuff to Discuss

I have some FIXMEs in the code for questions that came up along the way, as well as some others:

  • Is there any good way to detect whether something's droppable in the dock?
    • Just going to keep the Nuclide check for now and defer blessing anything without knowing the problem more
  • There are some known edge cases with showing the toggle buttons on hover. Should we address them?
    • Going to listen for leaving the central pane area, then handle it with mousemoves/position comparisons
  • There's code that remembers the last place you put an item so that, if you close it and later open another, it'll show in the same place.
    • Is trying to remember the split worth it? It's not enough to just do it when a pane is moved (as is currently happening) since another pane moving would affect it. We could abuse serialize() and do it there instead. (It shouldn't be in the serialized value though because we want it to be shared across workspaces.) That wouldn't be perfect either though, because if the same item was in different locations in two windows, we wouldn't know which was preferred. (Either way, we're still kinda making a "best guess" since split location isn't context-independent.)
      • Not worth it; leave it out for now
    • Is there a more efficient storage mechanism than putting stringified JSON in local storage?
      • Use StateStore (this means openItem() will have to return a promise)
  • The open API doesn't currently have a way to ensure a new item is created. (It always searches at least the active pane of the center.) Does that matter?
    • Going to ignore until we have a use case
  • Is it weird to have some of the workspace API methods (paneForURI(), paneForItem())* include the docks while others (getActivePane()) don't? Similarly, if searchAllPanes includes docks, what's the desired behavior when searchAllPanes is false? (Previously, I mistakenly thought it meant to search no panes, so there wasn't any question.)
    • paneForURI() and paneForItem() will include docks; active-related stuff won't. therefore, searchAllPanes = true will search docks, false (which only checks the active pane) won't
    • a workspace.getCenter() will be added and return an instance of the new WorkspaceCenter class, which trivially wraps workspace.paneContainer
  • Oops, looks like I dropped the commit that updated those in a rebase. I'll add them back, depending on how we answer this question.

matthewwithanm added a commit to matthewwithanm/deprecation-cop that referenced this pull request Mar 15, 2017

Wait for view to be added before making assertions
This was relying on the views being added after the activation promise
was resolved. Since atom/atom#13977 adds an async db lookup to the
`open()` path, that's not enough.

matthewwithanm added a commit to matthewwithanm/fuzzy-finder that referenced this pull request Mar 15, 2017

Only check panes in center
atom/atom#13977 adds multiple pane locations and updates the
`atom.workspace.getPanes()` API to return the panes from all of them.
This fixes the resultant test failures by only checking the panes in
the center.

matthewwithanm added a commit to matthewwithanm/markdown-preview that referenced this pull request Mar 15, 2017

Make test resilient to async opening of preview
While `atom.workspace.open()` was already async, there were some
situations where the item would be added synchronously (which this test
took advantage of). After atom/atom#13977, this is no longer the case.

matthewwithanm added a commit to matthewwithanm/markdown-preview that referenced this pull request Mar 15, 2017

Only examine center panes in tests
atom/atom#13977 updates the `atom.workspace.getPanes()` API to include
panes in additional containers. This commit updates the tests to
maintain the original behavior.

matthewwithanm added a commit to matthewwithanm/settings-view that referenced this pull request Mar 15, 2017

Wait for `open()` to complete before considering command done
This has been relying on the pane to be created synchronously when
dispatching the command (and calling `open()`), which would only happen
in certain conditions. After atom/atom#13977, `open()` is more reliably
async so we need to take that into account.

matthewwithanm added a commit to matthewwithanm/tree-view that referenced this pull request Mar 16, 2017

Only check panes in center
atom/atom#13977 adds multiple pane locations and updates the
`atom.workspace.getPanes()` API to return the panes from all of them.
This fixes the resultant test failures by only checking the panes in
the center.

matthewwithanm added a commit to matthewwithanm/wrap-guide that referenced this pull request Mar 16, 2017

Count the number of editors (not panes) when testing wrap guide count
atom/atom#13977 adds multiple pane locations and updates the
atom.workspace.getPanes() API to return the panes from all of them.
This fixes the resultant test failures by making assertions about the
number of text editors instead of the number of panes (which I think is
more true to the intent of the test).

matthewwithanm added a commit to matthewwithanm/fuzzy-finder that referenced this pull request Mar 16, 2017

Only check panes in center
atom/atom#13977 adds multiple pane locations and updates the
`atom.workspace.getPanes()` API to return the panes from all of them.
This fixes the resultant test failures by only checking the panes in
the center.

matthewwithanm added a commit to matthewwithanm/markdown-preview that referenced this pull request Mar 16, 2017

Only examine center panes in tests
atom/atom#13977 updates the `atom.workspace.getPanes()` API to include
panes in additional containers. This commit updates the tests to
maintain the original behavior.

matthewwithanm added a commit to matthewwithanm/tree-view that referenced this pull request Mar 16, 2017

Only check panes in center
atom/atom#13977 adds multiple pane locations and updates the
`atom.workspace.getPanes()` API to return the panes from all of them.
This fixes the resultant test failures by only checking the panes in
the center.

matthewwithanm added some commits Mar 11, 2017

Don't mutate list during iteration
I saw a situation where this was calling `destroy()` on `undefined`鈥
presumably because destroying one caused the list to be mutated
elsewhere and the indexes to shift.

matthewwithanm added a commit to matthewwithanm/atom that referenced this pull request Mar 18, 2017

Add `saveFocusedPaneItem()` and call it in "core:save" command
Also, do the same for "core:save-as" and `saveFocusedPaneItemAs()`.
This behavior change means that pane items in docks (atom#13977) will be
savable too.

@matthewwithanm matthewwithanm changed the title from [WIP] Docks to Docks Mar 18, 2017

facebook-github-bot added a commit to facebook/nuclide that referenced this pull request Mar 30, 2017

Pass view registry to pane container
Summary: atom/atom#13977 updates the PaneContainer constructor to require the view registry. This is a forwards-compatible change.

Reviewed By: hansonw

Differential Revision: D4800218

fbshipit-source-id: 9f751aa21c5ac1d37ae4ba021cc6e9ed84dba507

@maxbrunsfeld maxbrunsfeld merged commit 76538fc into master Mar 30, 2017

5 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@maxbrunsfeld

This comment has been minimized.

Show comment
Hide comment
@maxbrunsfeld

maxbrunsfeld Mar 30, 2017

Contributor

馃憦 Thanks for your hard work on this @matthewwithanm. This is a huge feature.

Contributor

maxbrunsfeld commented Mar 30, 2017

馃憦 Thanks for your hard work on this @matthewwithanm. This is a huge feature.

@maxbrunsfeld maxbrunsfeld deleted the fb-mdt-docks branch Mar 30, 2017

@maxbrunsfeld

This comment has been minimized.

Show comment
Hide comment
@maxbrunsfeld

maxbrunsfeld Mar 30, 2017

Contributor

@simurai What do you think of the behavior now: the dock tabs only appear if there is actually something in the dock. They also appear when you're dragging a tab, in case you want to drag it to the dock. Does this make it less distracting? We'll likely iterate a bit on the UX, but I feel like this is a good default.

Contributor

maxbrunsfeld commented Mar 30, 2017

@simurai What do you think of the behavior now: the dock tabs only appear if there is actually something in the dock. They also appear when you're dragging a tab, in case you want to drag it to the dock. Does this make it less distracting? We'll likely iterate a bit on the UX, but I feel like this is a good default.

facebook-github-bot added a commit to facebook/nuclide that referenced this pull request Mar 31, 2017

Rename panel locations to use new Atom names
Summary:
Changes the existing names to the ones used by atom/atom#13977

pane -> center
right-panel -> right
left-panel -> left
bottom-panel -> bottom

Reviewed By: jgebhardt

Differential Revision: D4714075

fbshipit-source-id: 5ee7607f816aa9201b94fa372dd06f8a8dece1f1

facebook-github-bot added a commit to facebook/nuclide that referenced this pull request Mar 31, 2017

Add compat layer for workspace views -> Atom workspace pane location 鈥
鈥ransition

Summary: If the new Atom API is available, use that instead. This paves the way for deleting the workspace-views package (once atom/atom#13977 makes it to stable).

Reviewed By: shushz

Differential Revision: D4714099

fbshipit-source-id: 050e90b7aa41cabb698aac765e2df62d82eb7c4c
@simurai

This comment has been minimized.

Show comment
Hide comment
@simurai

simurai Mar 31, 2017

Member

@maxbrunsfeld Does this make it less distracting?

Yeah, a little, but docks will probably not be empty that often? I actually felt bad, suggesting to remove the toggle buttons 馃槆 , but then I was like: "Well, it's just a suggestion, people don't have to agree." So yeah, I'm ok to keep the toggle buttons as a default and maybe I can try to add a config option to hide them in the One themes. Then it's more like the wrap-guide, have it for easy discoverability, but still let people remove it, if they like a more minimal UI.

@matthewwithanm Thanks so much for this. Really 鉂わ笍 how it lets you organize panels exactly how you like them.

Member

simurai commented Mar 31, 2017

@maxbrunsfeld Does this make it less distracting?

Yeah, a little, but docks will probably not be empty that often? I actually felt bad, suggesting to remove the toggle buttons 馃槆 , but then I was like: "Well, it's just a suggestion, people don't have to agree." So yeah, I'm ok to keep the toggle buttons as a default and maybe I can try to add a config option to hide them in the One themes. Then it's more like the wrap-guide, have it for easy discoverability, but still let people remove it, if they like a more minimal UI.

@matthewwithanm Thanks so much for this. Really 鉂わ笍 how it lets you organize panels exactly how you like them.

@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Mar 31, 2017

Contributor

screen shot 2017-03-31 at 3 06 36 pm

@simurai Should we tweak the styling of tabs inside docks to go full width? For some reason the empty space to the right of the tree-view's tab is feeling awkward to me. Also, want to do an icon for the tree-view's tab? BTW I'm calling the "tree view" the "project browser" in the UI because I think it conveys more information.

Contributor

nathansobo commented Mar 31, 2017

screen shot 2017-03-31 at 3 06 36 pm

@simurai Should we tweak the styling of tabs inside docks to go full width? For some reason the empty space to the right of the tree-view's tab is feeling awkward to me. Also, want to do an icon for the tree-view's tab? BTW I'm calling the "tree view" the "project browser" in the UI because I think it conveys more information.

@simurai

This comment has been minimized.

Show comment
Hide comment
@simurai

simurai Apr 1, 2017

Member

@nathansobo Should we tweak the styling of tabs inside docks to go full width?

Yeah, for the One themes it will be the new default: atom/one-light-ui#96. It actually goes full width for all tabs, also in the center. I'll merge it so people can try it out and see how it feels. There is still the option to only use the minimum width:

tab-sizing

We could also try to add it to atom/tabs, but all themes have their own tabs behaviour, so probably would get overridden anyways.

Member

simurai commented Apr 1, 2017

@nathansobo Should we tweak the styling of tabs inside docks to go full width?

Yeah, for the One themes it will be the new default: atom/one-light-ui#96. It actually goes full width for all tabs, also in the center. I'll merge it so people can try it out and see how it feels. There is still the option to only use the minimum width:

tab-sizing

We could also try to add it to atom/tabs, but all themes have their own tabs behaviour, so probably would get overridden anyways.

@simurai simurai referenced this pull request Apr 14, 2017

Open

Compact settings #935

1 of 1 task complete

zardra added a commit to zardra/nuclide that referenced this pull request Apr 14, 2017

Pass view registry to pane container
Summary: atom/atom#13977 updates the PaneContainer constructor to require the view registry. This is a forwards-compatible change.

Reviewed By: hansonw

Differential Revision: D4800218

fbshipit-source-id: 9f751aa21c5ac1d37ae4ba021cc6e9ed84dba507

zardra added a commit to zardra/nuclide that referenced this pull request Apr 14, 2017

Rename panel locations to use new Atom names
Summary:
Changes the existing names to the ones used by atom/atom#13977

pane -> center
right-panel -> right
left-panel -> left
bottom-panel -> bottom

Reviewed By: jgebhardt

Differential Revision: D4714075

fbshipit-source-id: 5ee7607f816aa9201b94fa372dd06f8a8dece1f1

zardra added a commit to zardra/nuclide that referenced this pull request Apr 14, 2017

Add compat layer for workspace views -> Atom workspace pane location 鈥
鈥ransition

Summary: If the new Atom API is available, use that instead. This paves the way for deleting the workspace-views package (once atom/atom#13977 makes it to stable).

Reviewed By: shushz

Differential Revision: D4714099

fbshipit-source-id: 050e90b7aa41cabb698aac765e2df62d82eb7c4c

@yochem yochem referenced this pull request May 22, 2017

Closed

Atom docks #184

jasonrudolph added a commit to atom/fuzzy-finder that referenced this pull request Sep 1, 2017

Remove obsolete shim for atom.workspace.getCenter()
Now that the latest stable version of Atom includes atom/atom#13977, we
no longer need this shim.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment