Skip to content
This repository has been archived by the owner on Jan 13, 2025. It is now read-only.

feat(top app bar): Add short top app bar always collapsed feature #2327

Merged
merged 8 commits into from
Mar 2, 2018
Merged

feat(top app bar): Add short top app bar always collapsed feature #2327

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
667361e
feat(top-app-bar): Add always collapsed feature to short top app bar
williamernest Feb 28, 2018
8455cfb
feat(top-app-bar): Update tests/demo
williamernest Mar 1, 2018
8a1a961
feat(top-app-bar): Update Readme
williamernest Mar 1, 2018
99b066b
feat(top-app-bar): Update demo names
williamernest Mar 1, 2018
47317a4
feat(top-app-bar): Update for comments
williamernest Mar 1, 2018
7593d0b
feat(top-app-bar): Update readme.
williamernest Mar 1, 2018
12528ab
feat(top-app-bar): Update for comments
williamernest Mar 2, 2018
3d2d231
Merge branch 'master' into feat/top-app-bar/short-always-closed
williamernest Mar 2, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 21 additions & 6 deletions demos/top-app-bar.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,16 +160,20 @@
<div class="demo-controls-container">
<h3>Demo Controls</h3>
<div>
<input type="checkbox" id="rtl-checkbox"/>
<label for="rtl-checkbox">RTL</label>
<input type="checkbox" id="rtl-checkbox"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

For some reason these are now double-nested?

<label for="rtl-checkbox">RTL</label>
</div>
<div>
<input type="checkbox" id="short-checkbox"/>
<label for="short-checkbox">Short</label>
<input type="checkbox" id="short-checkbox"/>
<label for="short-checkbox">Short</label>
</div>
<div>
<input type="checkbox" id="no-action-item-checkbox"/>
<label for="no-action-item-checkbox">No Action Item</label>
<input type="checkbox" id="no-action-item-checkbox"/>
<label for="no-action-item-checkbox">No Action Item</label>
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd move this one up before Short to keep the Short-related things together

</div>
<div>
<input type="checkbox" id="always-closed-checkbox" disabled/>
<label for="always-closed-checkbox">Always Closed (Short Only)&#x200E;</label>
</div>
</div>

Expand All @@ -188,6 +192,7 @@ <h3>Demo Controls</h3>
var shortCheckbox = document.getElementById('short-checkbox');
var noActionItemCheckbox = document.getElementById('no-action-item-checkbox');
var rtlCheckbox = document.getElementById('rtl-checkbox');
var alwaysClosedCheckbox = document.getElementById('always-closed-checkbox');
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: reorder these to match DOM?


appBarEl.addEventListener('MDCTopAppBar:nav', function() {
drawer.open = true;
Expand All @@ -203,7 +208,17 @@ <h3>Demo Controls</h3>
if (!this.checked) {
appBarEl.classList.remove('mdc-top-app-bar--short-collapsed');
appBarEl.classList.remove('mdc-top-app-bar--short-has-action-item');
alwaysClosedCheckbox.checked = false;
}

alwaysClosedCheckbox.disabled = !this.checked;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest adding a disabled label style like in our text field demo page, to make the disabled state more obvious.

});

alwaysClosedCheckbox.addEventListener('change', function() {
this.checked ? appBarEl.classList.add('mdc-top-app-bar--short-collapsed') : appBarEl.classList.remove('mdc-top-app-bar--short-collapsed');
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment as last time, move the ternary to within square brackets like what was done for shortCheckbox in the previous PR


appBar.destroy();
appBar = mdc.topAppBar.MDCTopAppBar.attachTo(appBarEl);
});

noActionItemCheckbox.addEventListener('change', function() {
Expand Down
16 changes: 16 additions & 0 deletions packages/mdc-top-app-bar/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,22 @@ Short top app bars should only be used with one action item:
</header>
```

Short top app bars can be configured to stay collapsed by applying the `mdc-top-app-bar--short-collapsed` before instantiating the component :
Copy link
Contributor

Choose a reason for hiding this comment

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

I might re-word "stay" to "always appear"


```html
<header class="mdc-top-app-bar mdc-top-app-bar--short mdc-top-app-bar--short-collapsed">
<div class="mdc-top-app-bar__row">
<section class="mdc-top-app-bar__section mdc-top-app-bar__section--align-start">
<a href="#" class="material-icons mdc-top-app-bar__navigation-icon">menu</a>
<span class="mdc-top-app-bar__title">Title</span>
</section>
<section class="mdc-top-app-bar__section mdc-top-app-bar__section--align-end" role="top-app-bar">
<a href="#" class="material-icons mdc-top-app-bar__icon" aria-label="Bookmark this page" alt="Bookmark this page">bookmark</a>
</section>
</div>
</header>
```

### JavaScript

```js
Expand Down
18 changes: 12 additions & 6 deletions packages/mdc-top-app-bar/foundation.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,14 +67,13 @@ class MDCTopAppBarFoundation extends MDCFoundation {
}

init() {
this.adapter_.registerNavigationIconInteractionHandler('click', this.navClickHandler_);

const isShortTopAppBar = this.adapter_.hasClass(cssClasses.SHORT_CLASS);

if (isShortTopAppBar) {
this.adapter_.registerScrollHandler(this.scrollHandler_);
this.initShortAppBar_();
this.initShortTopAppBar_();
}

this.adapter_.registerNavigationIconInteractionHandler('click', this.navClickHandler_);
}

destroy() {
Expand All @@ -85,14 +84,21 @@ class MDCTopAppBarFoundation extends MDCFoundation {
/**
* Used to set the initial style of the short top app bar
*/
initShortAppBar_() {
initShortTopAppBar_() {
const isAlwaysCollapsed = this.adapter_.hasClass(cssClasses.SHORT_COLLAPSED_CLASS);

if (this.adapter_.getTotalActionItems() > 0) {
this.adapter_.addClass(cssClasses.SHORT_HAS_ACTION_ITEM_CLASS);
}

if (!isAlwaysCollapsed) {
this.adapter_.registerScrollHandler(this.scrollHandler_);
this.shortAppBarScrollHandler_();
}
}

/**
* Scroll handler for applying/removing the closed modifier class
* Scroll handler for applying/removing the collapsed modifier class
Copy link
Contributor

Choose a reason for hiding this comment

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

Hey I missed this one last time didn't I 👀

* on the short top app bar.
*/
shortAppBarScrollHandler_() {
Expand Down
16 changes: 15 additions & 1 deletion test/unit/mdc-top-app-bar/foundation.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,15 +93,27 @@ test('short top app bar: scroll listener is removed on destroy', () => {
td.verify(mockAdapter.deregisterScrollHandler(td.matchers.isA(Function)), {times: 1});
});

test('short top app bar: scroll listener is not registered if collapsed class exists before init', () => {
const {foundation, mockAdapter} = setupTest();
td.when(mockAdapter.hasClass(MDCTopAppBarFoundation.cssClasses.SHORT_CLASS)).thenReturn(true);
td.when(mockAdapter.hasClass(MDCTopAppBarFoundation.cssClasses.SHORT_COLLAPSED_CLASS)).thenReturn(true);
td.when(mockAdapter.getTotalActionItems()).thenReturn(0);
foundation.init();
td.verify(mockAdapter.registerScrollHandler(td.matchers.anything()), {times: 0});
});

test('short top app bar: class is added once when page is scrolled from the top', () => {
const {foundation, mockAdapter} = setupTest();
const mockRaf = createMockRaf();

td.when(mockAdapter.hasClass(MDCTopAppBarFoundation.cssClasses.SHORT_CLASS)).thenReturn(true);
td.when(mockAdapter.hasClass(MDCTopAppBarFoundation.cssClasses.SHORT_COLLAPSED_CLASS)).thenReturn(false);
td.when(mockAdapter.getTotalActionItems()).thenReturn(0);
td.when(mockAdapter.getViewportScrollY()).thenReturn(0);

const {scrollHandler} = createMockHandlers(foundation, mockAdapter, mockRaf);

td.when(mockAdapter.getViewportScrollY()).thenReturn(1);

scrollHandler();
scrollHandler();

Expand All @@ -113,6 +125,8 @@ test('short top app bar: class is removed once when page is scrolled to the top'
const mockRaf = createMockRaf();

td.when(mockAdapter.hasClass(MDCTopAppBarFoundation.cssClasses.SHORT_CLASS)).thenReturn(true);
td.when(mockAdapter.hasClass(MDCTopAppBarFoundation.cssClasses.SHORT_COLLAPSED_CLASS)).thenReturn(false);
td.when(mockAdapter.getTotalActionItems()).thenReturn(0);

const {scrollHandler} = createMockHandlers(foundation, mockAdapter, mockRaf);
// Apply the closed class
Expand Down
12 changes: 12 additions & 0 deletions test/unit/mdc-top-app-bar/util.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,3 +40,15 @@ test('applyPassive returns false for browsers that do not support passive event
};
assert.isFalse(util.applyPassive(mockWindow, true));
});

test('applyPassive returns previous value when called twice without refresh', () => {
const mockWindow = {
document: {
addEventListener: function(name, method, options) {
return options.passive;
},
},
};
util.applyPassive(mockWindow, true);
assert.deepEqual(util.applyPassive(mockWindow, false), {passive: true});
Copy link
Contributor

Choose a reason for hiding this comment

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

This is kind of tangential to this PR anyway, but is this really testing what we need it to test? I think this would pass whether we called it with force=true or false. (Also force=true mainly exists for tests to begin with.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added this to improve branch coverage to 100%. However, after referencing mdc-ripple I'll add applyPassive to the deregister adapter method so that it gets his twice in a test.

});