Create tabbed interface for GitHub features by BinaryMuse · Pull Request #461 · atom/github

BinaryMuse · 2017-01-20T00:47:49Z

No description provided.

BinaryMuse · 2017-01-20T01:11:51Z

 .github-PanelResizer {
  // ok, yeah, magic number.. err color
-  background-color: mix(@base-background-color, @tool-panel-background-color);
+  // background-color: mix(@base-background-color, @tool-panel-background-color);


@simurai

I'm having some problems with this. Why is it that when I move this line........

BinaryMuse · 2017-01-20T01:12:00Z

+    flex: 1;
+    display: flex;
+    flex-direction: column;
+    // background-color: mix(@base-background-color, @tool-panel-background-color);


.......to here, the color looks wrong?

Yeah, the color mix() is kinda custom. So either we could use it also in .github-Tabs-NavigationItem .active. Or use @tab-background-color-active for the .github-Tabs-PanelContainer.

I can play around with it, if you like.

@simurai That'd be great!! I'd also love to understand what I was doing wrong xD

Basically I was trying to get the background behind the tabs to match the background behind the tree-view/tabs, and the rest of the panel to have your original BG color

If you uncomment here ☝️ .

BinaryMuse · 2017-01-20T01:42:06Z

@simurai you'll need to run this to make the tabbed interface show up atom.config.set('github.githubEnabled', true)

smashwilson

Looking 💯 so far 😁

smashwilson · 2017-01-20T02:01:47Z

+        </nav>
+        <section className="github-Tabs-PanelContainer">
+          {children.map((child, i) => (
+            child && React.cloneElement(child, {active: i === this.props.activeIndex})


Do these cloned elements need a key?

I think toArray takes care of adding them in this case

smashwilson · 2017-01-20T02:06:28Z

+}
+
+Tabs.Panel = TabPanel;
+export default Tabs;


Any reason not to export default class Tabs { ... } up above?

For that matter I think you can do:

Tabs.Panel = class TabPanel extends React.Component { // ... }

Just a style thing, but I think elsewhere we've tended to export at declaration site. I tend to prefer that myself because it makes it evident how to import a definition right at the declaration site without needing to hunt for the exports.

smashwilson · 2017-01-20T02:07:30Z

+              </EtchWrapper>
+            </Tabs.Panel>
+            {this.props.githubEnabled && (
+              <Tabs.Panel title="Hub">


Hee hee, I still think that's great.

Ya, let's leave it for a bit longer. 😂

smashwilson · 2017-01-20T02:08:48Z

        repository={this.getActiveRepository()}
        statusBar={this.statusBar}
        savedState={this.savedState.gitController}
+        githubEnabled={this.config.get('github.githubEnabled')}


Feature flags ftw ✨

simurai · 2017-01-20T04:19:16Z

+      border: 1px solid @tab-border-color;
+      border-left: 2px solid @background-color-info;
+      border-bottom: none;
+      background: @tab-background-color-active;


...and change ☝️ to background-color: mix(@base-background-color, @tool-panel-background-color);, then it should match.

But maybe panel tabs, shouldn't look like regular editor tabs. So that it doesn't feel like they are part of the tab-bar and would open a pane.

Maybe something more lightweight with just borders? Similar to the tabs here on github.com.

simurai · 2017-01-20T08:58:40Z

Currently the tabs look like:

But not sure, it feels uncanny. Also, because themes style the editor tabs (instead of the tabs package), it will be off/un-aligned for most themes. And pretty different for some. Here the atom-light-ui 🙈 :

Hmm.. either we use the exact same markup + class names as the tabs package or we try to make it look less like tabs. Then it might be ok that they are different.

Here a version with underlines for the selection:

Or use a "segmented control":

Or use vertical tabs, like the go-plus package:

I think I like the vertical tabs best. Also because there would be tons of space for more tabs. Like we wouldn't have to cram everything into just two panels but could have lots more. Downside is that it's limited to icons only. Or maybe with label if they are not too long.

BinaryMuse · 2017-01-20T16:13:57Z

Oh good call. I like the last two options. I say we try vertical tabs and see how it feels.

BinaryMuse · 2017-01-20T16:15:49Z

Hm the other downside to vertical tabs is you lose horizontal space all the way down the pane, instead of space just at the top...

smashwilson · 2017-01-20T16:19:31Z

I kind of like the segmented control, personally.

I can't think of any other tabs we'd need to leave space for, and we could always change to vertical tabs if and when we need them.

BinaryMuse · 2017-01-20T20:38:17Z

I experimented with the vertical tab list but it felt super weird... with only two icons there's a tooon of wasted space. I'm gonna go for the segmented UI for now and we can go from there if we want to change it.

BinaryMuse · 2017-01-20T23:39:01Z

@simurai Would you mind taking a quick look at the most recent changes (mostly in 8d7d094) and changing anything as you see fit? In particular, I wasn't sure what .github-Tabs-NavigationItem.active.color should be.

simurai · 2017-01-21T00:13:31Z

@BinaryMuse We probably could use these "Selected buttons" from the styleguide. Then the selected color will be set by the themes.

<div class='btn-group'>
  <button class='btn selected'>Git</button>
  <button class='btn'>Hub</button>
</div>

BinaryMuse · 2017-01-21T00:16:08Z

@simurai The downside there is we have to change the Tab panel (which I plan on extracting) to use the button groups — but if that's really what makes the most sense here, I could make that work.

simurai · 2017-01-21T00:22:56Z

By "change the Tab panel", you mean change the markup and class names? Yeah, right.

If you wanna keep it custom, we can try using @button-background-color-selected for the selected background.

simurai · 2017-01-21T01:07:39Z

@BinaryMuse Ah, sorry, never mind. You're already using @button-background-color-selected. I guess you meant the (text) color. That's more tricky because it's hard to guess the background color. Less has contrast() function. I'll try to add that.

So it works for light and dark themes

So that the height still looks the same

So it matches other stuff like buttons or inputs. Also I think the `:not()` selectors aren't really needed.

So it matches themes

In case they overflow, one day.

Michelle Tilley added 2 commits January 19, 2017 14:33

again

f57bfea

Implement those tabs ✨

a8f1d09

BinaryMuse commented Jan 20, 2017

View reviewed changes

Only display github panel if github.githubEnabled config is true

a055aef

BinaryMuse requested a review from smashwilson January 20, 2017 01:43

smashwilson reviewed Jan 20, 2017

View reviewed changes

🎨 per @smashwilson

60a4b77

simurai reviewed Jan 20, 2017

View reviewed changes

Style tabs

bb8642e

Michelle Tilley added 4 commits January 20, 2017 15:34

Allow TabPanel title to be a node

364b0f6

Don't copy title prop onto Tab Panel DOM node

0bf5a03

Change Tab conditional rendering to fix linter format

39995ed

Display tabs navigation as a segmented button group

8d7d094

simurai added 5 commits January 21, 2017 11:18

Add contrast to labels

99cc35a

So it works for light and dark themes

Show border also when active

2dc7fb1

So that the height still looks the same

Use variable for corner radius

b978b9c

So it matches other stuff like buttons or inputs. Also I think the `:not()` selectors aren't really needed.

Use variable for padding

3073d62

So it matches themes

Add ellipsis

c8cb7a3

In case they overflow, one day.

BinaryMuse merged commit d05d13b into master Jan 21, 2017

BinaryMuse deleted the mkt-ku-add-tabbed-interface branch January 21, 2017 03:38

BinaryMuse mentioned this pull request Feb 6, 2017

Separate tabs for Git/GitHub in panel #266

Closed

Conversation

BinaryMuse commented Jan 20, 2017

Uh oh!

BinaryMuse Jan 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BinaryMuse commented Jan 20, 2017

Uh oh!

smashwilson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

simurai Jan 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

simurai commented Jan 20, 2017

Uh oh!

BinaryMuse commented Jan 20, 2017

Uh oh!

BinaryMuse commented Jan 20, 2017

Uh oh!

smashwilson commented Jan 20, 2017

Uh oh!

BinaryMuse commented Jan 20, 2017

Uh oh!

BinaryMuse commented Jan 20, 2017

Uh oh!

simurai commented Jan 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BinaryMuse commented Jan 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

simurai commented Jan 21, 2017

Uh oh!

simurai commented Jan 21, 2017

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

BinaryMuse Jan 20, 2017 •

edited

Loading

simurai Jan 20, 2017 •

edited

Loading

simurai commented Jan 21, 2017 •

edited

Loading

BinaryMuse commented Jan 21, 2017 •

edited

Loading