feat(tab-nav-bar): add initial functionality of tab nav bar #1589

andrewseguin · 2016-10-24T23:19:29Z

Initial development of the tab nav bar

Closes #524

jelbourn · 2016-10-24T23:56:29Z

src/demo-app/tabs/routes.ts

+
+import {RoutedContent1, RoutedContent2, RoutedContext3} from '../tabs/tabs-demo';
+
+export const TABS_DEMO_ROUTE: Routes = [


TABS_DEMO_ROUTES (plural)

jelbourn · 2016-10-24T23:58:00Z

src/demo-app/tabs/routes.ts

+  { path: '', redirectTo: 'content-1', pathMatch: 'full' },
+  { path: 'content-1', component: RoutedContent1 },
+  { path: 'content-2', component: RoutedContent2 },
+  { path: 'content-3', component: RoutedContext3 },


I'd use a different name for the routed content components to be super clear that they're examples and that people don't have to call their components "RoutedContentX". Popular examples are food, weather, pokemon...

jelbourn · 2016-10-25T00:07:33Z

src/lib/tabs/tab-nav-bar/tab-nav-bar.ts

@@ -0,0 +1,40 @@
+import { Component, Input, ViewChild, ElementRef } from '@angular/core';
+import { MdInkBar } from '../ink-bar';


No spaces inside imports (this might be a WebStorm settings)

jelbourn · 2016-10-25T00:08:07Z

src/lib/tabs/tab-nav-bar/tab-nav-bar.ts

+  styleUrls: ['tab-nav-bar.css'],
+})
+export class MdTabNavBar {
+  @ViewChild(MdInkBar) private _inkBar: MdInkBar;


@ViewChild can't be applied to a private member (since Angular itself is external to this class and has to touch it).

jelbourn · 2016-10-25T00:08:33Z

src/lib/tabs/tab-nav-bar/tab-nav-bar.ts

+export class MdTabNavBar {
+  @ViewChild(MdInkBar) private _inkBar: MdInkBar;
+
+  updateActiveLink(element: HTMLElement) {


Add function description comment

jelbourn · 2016-10-25T00:09:21Z

src/lib/tabs/tab-nav-bar/tab-nav-bar.ts

+import { Component, Input, ViewChild, ElementRef } from '@angular/core';
+import { MdInkBar } from '../ink-bar';
+
+@Component({


Add class description for what this component does (since it's not one of the totally obvious ones like radio-button).

jelbourn · 2016-10-25T00:11:39Z

src/lib/tabs/tabs-common.scss

+$md-tab-bar-height: 48px !default;
+
+
+@mixin tab-label {


Add comment explaining what the mixin is for

jelbourn · 2016-10-25T00:13:19Z

src/lib/tabs/tab-nav-bar/tab-nav-bar.html

@@ -0,0 +1,4 @@
+<div class="md-tab-header">


I'm thinking that this should probably be a <nav> element instead of a div.

@marcysutton Is it appropriate to always use a <nav> element for a component that contains anchors and navigates the page, or should <nav> only be used for major site navigation?

Help me level-set: is the set of anchors visually like tabs, but without the associated tabpanels and ARIA tab semantics/keyboard behavior?

The difference between a <div> and <nav> is that the latter has a landmark role, making it easy for a screen reader user to discover. I think this component would be a reasonable use case for how I've seen tabs presented in the Material Design spec. Unless you anticipate so many of them on a page they dominate the landmarks list, I don't see much of a downside.

If you do end up using <nav>, I'd recommend labeling the element with an aria-label so it can be easily identified as a screen reader landmark.

Edit: I thought I had sent this comment days ago, but I'm still not used to the Review workflow apparently.

Thanks for the input Marcy. Jeremy and I discussed this and agree that we can move this responsibility to the user by making this a component with an attribute selector.

Usage of this component will be written by the user as such:
<nav md-tab-nav-bar aria-label="navigation links"> ... </nav>

jelbourn · 2016-10-25T00:27:17Z

src/lib/tabs/tab-nav-bar/tab-nav-bar.ts

+  private _isActive: boolean = false;
+
+  @Input()
+  get active(): boolean {


I didn't know about this before, but looking through the router docs turned up this:
https://angular.io/docs/ts/latest/guide/router.html#!#router-link-active

Seems like we could rely on that to apply the appropriate class based on the route being active

Adding a comment that addresses the workaround in our demo app because we lack an isActive api on routerLinks

jelbourn

Just a few more things I noticed

jelbourn · 2016-10-26T00:31:23Z

src/demo-app/tabs/routes.ts

+  { path: '', redirectTo: 'sunny-tab', pathMatch: 'full' },
+  { path: 'sunny-tab', component: SunnyTabContent },
+  { path: 'rainy-tab', component: RainyTabContent },
+  { path: 'foggy-tab', component: FoggyTabContent },


Remove the leading/trailing spaces in object literals

jelbourn · 2016-10-26T00:33:50Z

src/lib/tabs/tab-nav-bar/tab-nav-bar.scss

+  display: block;
+}
+
+a[md-tab-link] {


I'd remove the a part of this selector just to reduce the specificity.

jelbourn · 2016-10-26T00:40:45Z

src/lib/tabs/tabs-common.scss

@@ -0,0 +1,51 @@
+@import '../core/style/variables';


This file should either consist of exclusively mixins or exclusively concrete styles.

If it's the former, then the file name should start with an underscore (which tells the sass compiler that it's a "partial" file that doesn't have output).

If it's the latter, it should be included in the "styleUrls" for each of the components that use it (rather than doing a sass @import)

andrewseguin · 2016-10-26T17:37:34Z

Thanks for the review, I made the tabs common to be purely mixins

jelbourn · 2016-10-26T21:54:17Z

LGTM- nice work!

angular-automatic-lock-bot · 2019-09-06T03:01:43Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

andrewseguin added 2 commits October 24, 2016 16:04

feat(tab-nav-bar): add initial functionality of tab nav bar

3d221b2

style(tab-nav-bar): cleanup lint issues

77d7a7d

googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Oct 24, 2016

andrewseguin added the pr: needs review label Oct 24, 2016

andrewseguin assigned jelbourn Oct 24, 2016

jelbourn reviewed Oct 25, 2016

View reviewed changes

jelbourn mentioned this pull request Oct 25, 2016

WIP: feat(tab-nav-bar): initial implementation #1024

Closed

5 tasks

andrewseguin added 4 commits October 25, 2016 09:57

style(tab-nav-bar): rename tab demo routes

559989d

style(tab-nav-bar): Add comments and remove private view child

4b006b0

style(tab-nav-bar): Fix nav bar comments

907ecdc

chore(tab-nav-bar): addressing comments

42b8ba8

jelbourn reviewed Oct 26, 2016

View reviewed changes

fix styles

243b349

andrewseguin added 3 commits October 26, 2016 10:45

revert tooltip

27bb066

move nav/aria concerns out of the nav bar

4334bb0

fix unit test

d808c71

jelbourn added pr: lgtm and removed pr: needs review labels Oct 26, 2016

jelbourn merged commit 572b36e into angular:master Oct 26, 2016

andrewseguin deleted the tabs-dev branch October 27, 2016 22:19

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 6, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(tab-nav-bar): add initial functionality of tab nav bar #1589

feat(tab-nav-bar): add initial functionality of tab nav bar #1589

andrewseguin commented Oct 24, 2016 •

edited

Loading

jelbourn Oct 24, 2016

andrewseguin Oct 25, 2016

jelbourn Oct 24, 2016

andrewseguin Oct 25, 2016

jelbourn Oct 25, 2016

andrewseguin Oct 25, 2016

jelbourn Oct 25, 2016

andrewseguin Oct 25, 2016

jelbourn Oct 25, 2016

andrewseguin Oct 25, 2016

jelbourn Oct 25, 2016

andrewseguin Oct 25, 2016

jelbourn Oct 25, 2016

jelbourn Oct 25, 2016

marcysutton Oct 25, 2016 •

edited

Loading

andrewseguin Oct 26, 2016

jelbourn Oct 25, 2016

andrewseguin Oct 25, 2016

jelbourn left a comment

jelbourn Oct 26, 2016

jelbourn Oct 26, 2016

jelbourn Oct 26, 2016

andrewseguin commented Oct 26, 2016

jelbourn commented Oct 26, 2016

angular-automatic-lock-bot bot commented Sep 6, 2019


		import {RoutedContent1, RoutedContent2, RoutedContext3} from '../tabs/tabs-demo';

		export const TABS_DEMO_ROUTE: Routes = [

		@@ -0,0 +1,40 @@
		import { Component, Input, ViewChild, ElementRef } from '@angular/core';
		import { MdInkBar } from '../ink-bar';

feat(tab-nav-bar): add initial functionality of tab nav bar #1589

feat(tab-nav-bar): add initial functionality of tab nav bar #1589

Conversation

andrewseguin commented Oct 24, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcysutton Oct 25, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jelbourn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewseguin commented Oct 26, 2016

jelbourn commented Oct 26, 2016

angular-automatic-lock-bot bot commented Sep 6, 2019

andrewseguin commented Oct 24, 2016 •

edited

Loading

marcysutton Oct 25, 2016 •

edited

Loading