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

Show tab bar if the setting is true or there is more than one tab #539

Merged
merged 16 commits into from Jan 16, 2019

Conversation

Projects
None yet
2 participants
@50Wliu
Copy link
Member

50Wliu commented May 13, 2018

Requirements

  • Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
  • All new code requires tests to ensure against regressions

Description of the Change

This PR changes updateTabBarVisibility to hide the tab bar if there is less than two tabs, regardless of how many panes are open (as the tab bar is per-pane).

Example:
tab-bar-visibility

I inlined shouldAllowDrag as it was only being used by updateTabBarVisibility, and not even for dragging purposes.

This probably won't be merged until I also implement #275 (comment), to easier facilitate drag-and-dropping.
See GIF!
tab-bar-appearance-drag-and-drop

Along the way, I also fixed some very incorrect test assertions about the alwaysShowTabBar setting.

Alternate Designs

None.

Benefits

More consistent experience when "Always Show Tab Bar" is disabled.

Possible Drawbacks

This removes the ability to drag tabs out of a pane that only has one tab.

Applicable Issues

Fixes #275
Inspired by #277
Needs atom/tree-view#1293 to work correctly with the Tree View

50Wliu added some commits May 13, 2018

50Wliu added some commits Dec 20, 2018

@50Wliu 50Wliu removed the needs-testing label Dec 20, 2018

50Wliu added some commits Dec 20, 2018

50Wliu added some commits Dec 20, 2018

50Wliu added some commits Dec 20, 2018

@daviwil

daviwil approved these changes Jan 8, 2019

Copy link
Member

daviwil left a comment

Looks good! Just one question

@@ -230,15 +237,12 @@ class TabBarView
getWindowId: ->
@windowId ?= atom.getCurrentWindow().id

shouldAllowDrag: ->
(@paneContainer.getPanes().length > 1) or (@pane.getItems().length > 1)

This comment has been minimized.

@daviwil

daviwil Jan 8, 2019

Member

Looks like there was logic for pane containers here too, how will they be affected by this change?

This comment has been minimized.

@50Wliu

50Wliu Jan 9, 2019

Author Member

The logic for pane containers was what caused #275 to begin with. It made the tab bar always show if there was more than one pane container present. I also addressed the downside removing this causes in the Possible Drawbacks section, which is that it removes the ability to drag tabs out of a pane that only has one tab.

This comment has been minimized.

@daviwil

daviwil Jan 15, 2019

Member

Cool, that sounds fine to me!

@daviwil

This comment has been minimized.

Copy link
Member

daviwil commented Jan 15, 2019

Hey @50Wliu, looks like a recent commit you made created a merge conflict that needs to be reconciled with the changes in this PR:

d6947e3

If you can update the PR to take care of that conflict, feel free to merge it afterward. Thanks a bunch!

50Wliu added some commits Jan 16, 2019

🐛
updateTabBarVisibility is called other places as well

@50Wliu 50Wliu merged commit 7f677cd into master Jan 16, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@50Wliu 50Wliu deleted the wl-always-show-tab-bar branch Jan 16, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.