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

fix(top-app-bar): foundation should be destroyed and reinitialized on variant change #519

Merged
20 changes: 18 additions & 2 deletions packages/top-app-bar/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -71,10 +71,25 @@ export default class TopAppBar extends React.Component {
this.foundation_.destroy();
}

componentDidUpdate(prevProps) {
const foundationChanged = ['short', 'shortCollapsed', 'fixed']
.some((variant) => this.props[variant] !== prevProps[variant] );
Copy link

Choose a reason for hiding this comment

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

formatting issue, but for the sake of time I'm going to ignore this one and fix in a following pr


if (foundationChanged) {
this.initializeFoundation();
}
}


initializeFoundation = () => {
if (this.props.short) {
const {short, shortCollapsed, fixed} = this.props;
if (this.foundation_) {
this.foundation_.destroy();
}

if (short || shortCollapsed) {
this.foundation_ = new MDCShortTopAppBarFoundation(this.adapter);
} else if (this.props.fixed) {
} else if (fixed) {
this.foundation_ = new MDCFixedTopAppBarFoundation(this.adapter);
} else {
this.foundation_ = new MDCTopAppBarFoundation(this.adapter);
Expand All @@ -83,6 +98,7 @@ export default class TopAppBar extends React.Component {
this.foundation_.init();
}


addClassesToElement(classes, element) {
const updatedProps = {
className: classnames(classes, element.props.className),
Expand Down
3 changes: 2 additions & 1 deletion test/screenshot/golden.json
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
"top-app-bar/standard": "7a2dd6318d62ac2eabd66f1b28100db7c15840ccb753660065fa9524db6435d6",
"top-app-bar/standardNoActionItems": "6d361edb994cafcc6ac720336d12ee7d7114745993e16abd6e6b00f078424ff2",
"top-app-bar/standardWithNavigationIconElement": "95afd559c35dede805e4d4b51ad1fabd82b4117c358a8679e3166b88e059bf68",
"top-app-bar/prominentToShortCollapsed": "ce23a6060af19f93ee0255e45ebe51c7243ff12c14b2c09d070501ea7806e316",
"top-app-bar/dense": "e273e6c777f168248f5757c1f00a74206f4cce51c484d91cc7d36dc16de7d0de",
"top-app-bar/prominentDense": "cc8af934f9187ffd8f250834ef7c73e5c53c5ace10126bb855f74878ba125149",
"drawer/permanentBelowTopAppBar": "587ee2605c4b3e3f0408c6130b824b58587e05cedf9b964e14fc481b9de1e4c2",
Expand All @@ -41,4 +42,4 @@
"drawer/modal": "da83487c9349b253f7d4de01f92d442de55aab92a8028b77ff1a48cfaa265b72",
"drawer/permanentToModal": "6355905c2241b5e6fdddc2e25119a1cc3b062577375a88b59e6750c4b76e4561",
"typography": "c5e87d672d8c05ca3b61c0df4971eabe3c6a6a1f24a9b98f71f55a23360c498a"
}
}
60 changes: 60 additions & 0 deletions test/screenshot/top-app-bar/prominentToShortCollapsed.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
import React from 'react';
import TopAppBar from '../../../packages/top-app-bar';
import MaterialIcon from '../../../packages/material-icon';
import MainTopAppBarContent from './mainContent';

class TopAppBarProminentToShortCollapsedScreenshotTest extends React.Component {
state = {
isPhone: window.innerWidth < 599,
};

componentDidMount() {
window.addEventListener('resize', this.updateTopAppBarVariant);
}

shouldComponentUpdate(_, nextState) {
return nextState.isPhone !== this.state.isPhone;
}

componentWillUnmount() {
window.removeEventListener('resize', this.updateTopAppBarVariant);
}

updateTopAppBarVariant = () => {
const isPhone = window.innerWidth < 599;
this.setState({isPhone});
}

render() {
if (this.state.isPhone) {
return (
<div>
<TopAppBar
shortCollapsed
navigationIcon={<MaterialIcon
icon='menu'
onClick={() => console.log('click')}
/>}
/>
<MainTopAppBarContent />
</div>
);
}

return (
<div>
<TopAppBar
prominent
title={'Annie, I\'m a Hawk'}
navigationIcon={<MaterialIcon
icon='menu'
onClick={() => console.log('click')}
/>}
/>
<MainTopAppBarContent />
</div>
);
}
}

export default TopAppBarProminentToShortCollapsedScreenshotTest;
1 change: 1 addition & 0 deletions test/screenshot/top-app-bar/variants.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,4 +8,5 @@ export default [
'standard',
'standardNoActionItems',
'standardWithNavigationIconElement',
'prominentToShortCollapsed',
];
105 changes: 105 additions & 0 deletions test/unit/top-app-bar/index.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,111 @@ test('#enableRippleOnElement throws error if a native element', () => {
);
});

test('when changes from short to fixed the foundation changes', () => {
Copy link

Choose a reason for hiding this comment

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

wow great test coverage!

const wrapper = shallow(<TopAppBar short />);
const originalFoundation = wrapper.instance().foundation_;
wrapper.setProps({fixed: true, short: false});
assert.notEqual(wrapper.instance().foundation_, originalFoundation);
assert.exists(wrapper.instance().foundation_);
});

test('when changes from short to fixed the foundation changes', () => {
const wrapper = shallow(<TopAppBar short />);
const originalFoundation = wrapper.instance().foundation_;
wrapper.setProps({fixed: true, short: false});
assert.notEqual(wrapper.instance().foundation_, originalFoundation);
assert.exists(wrapper.instance().foundation_);
});

test('when changes from short to standard the foundation changes', () => {
const wrapper = shallow(<TopAppBar short />);
const originalFoundation = wrapper.instance().foundation_;
wrapper.setProps({short: false});
assert.notEqual(wrapper.instance().foundation_, originalFoundation);
assert.exists(wrapper.instance().foundation_);
});

test('when changes from short to prominent the foundation changes', () => {
const wrapper = shallow(<TopAppBar short />);
const originalFoundation = wrapper.instance().foundation_;
wrapper.setProps({short: false, prominent: true});
assert.notEqual(wrapper.instance().foundation_, originalFoundation);
assert.exists(wrapper.instance().foundation_);
});

test('when changes from short to shortCollpased the foundation changes', () => {
Copy link

Choose a reason for hiding this comment

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

Shouldn't the foundation not change in this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From what I understand it should reinitialize. When init() is called on MDCShortTopAppBarFoundation it registers a resize handler if initialized as a short variant. If it begins life collapsed it would not be necessary to register the resize handler. [0] Alternatively the resize handler could simply be deregistered. But I think this would work just as well?

[0] https://github.com/material-components/material-components-web/blob/a487783c74a42104a27ed6de42bad9d6d29a0a63/packages/mdc-top-app-bar/short/foundation.js#L44-L56

Copy link

Choose a reason for hiding this comment

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

ah right - this is because you would want the short collapsed to immediately collapse. Good call. Thanks!

const wrapper = shallow(<TopAppBar short />);
const originalFoundation = wrapper.instance().foundation_;
wrapper.setProps({shortCollapsed: true});
assert.notEqual(wrapper.instance().foundation_, originalFoundation);
assert.exists(wrapper.instance().foundation_);
});

test('when changes from fixed to prominent the foundation changes', () => {
const wrapper = shallow(<TopAppBar fixed/>);
const originalFoundation = wrapper.instance().foundation_;
wrapper.setProps({fixed: false, prominent: true});
assert.notEqual(wrapper.instance().foundation_, originalFoundation);
assert.exists(wrapper.instance().foundation_);
});

test('when changes from fixed to short the foundation changes', () => {
const wrapper = shallow(<TopAppBar fixed/>);
const originalFoundation = wrapper.instance().foundation_;
wrapper.setProps({fixed: false, short: true});
assert.notEqual(wrapper.instance().foundation_, originalFoundation);
assert.exists(wrapper.instance().foundation_);
});

test('when changes from fixed to shortCollpased the foundation changes', () => {
const wrapper = shallow(<TopAppBar fixed/>);
const originalFoundation = wrapper.instance().foundation_;
wrapper.setProps({fixed: false, shortCollapsed: true});
assert.notEqual(wrapper.instance().foundation_, originalFoundation);
assert.exists(wrapper.instance().foundation_);
});

test('when changes from fixed to standard the foundation changes', () => {
const wrapper = shallow(<TopAppBar fixed/>);
const originalFoundation = wrapper.instance().foundation_;
wrapper.setProps({fixed: false});
assert.notEqual(wrapper.instance().foundation_, originalFoundation);
assert.exists(wrapper.instance().foundation_);
});

test('when changes from standard to fixed the foundation changes', () => {
const wrapper = shallow(<TopAppBar />);
const originalFoundation = wrapper.instance().foundation_;
wrapper.setProps({fixed: true});
assert.notEqual(wrapper.instance().foundation_, originalFoundation);
assert.exists(wrapper.instance().foundation_);
});

test('when changes from standard to short the foundation changes', () => {
const wrapper = shallow(<TopAppBar />);
const originalFoundation = wrapper.instance().foundation_;
wrapper.setProps({short: true});
assert.notEqual(wrapper.instance().foundation_, originalFoundation);
assert.exists(wrapper.instance().foundation_);
});

test('when changes from standard to shortCollapsed the foundation changes', () => {
const wrapper = shallow(<TopAppBar />);
const originalFoundation = wrapper.instance().foundation_;
wrapper.setProps({shortCollapsed: true});
assert.notEqual(wrapper.instance().foundation_, originalFoundation);
assert.exists(wrapper.instance().foundation_);
});

test('when changes from standard to prominent the foundation does not ' +
'change', () => {
const wrapper = shallow(<TopAppBar />);
const originalFoundation = wrapper.instance().foundation_;
wrapper.setProps({prominent: true});
assert.equal(wrapper.instance().foundation_, originalFoundation);
assert.exists(wrapper.instance().foundation_);
});

test('#componentWillUnmount destroys foundation', () => {
const wrapper = shallow(<TopAppBar />);
const foundation = wrapper.instance().foundation_;
Expand Down