Skip to content

Commit

Permalink
clarify purpose of shouldComponentUpdate and test code
Browse files Browse the repository at this point in the history
  • Loading branch information
@chandlerprall
chandlerprall committed May 1, 2018
1 parent 23af5c7 commit 2362caf
Show file tree
Hide file tree
Showing 3 changed files with 72 additions and 106 deletions.
24 changes: 8 additions & 16 deletions src/components/context_menu/__snapshots__/context_menu_panel.test.js.snap
View file
Original file line number Diff line number Diff line change
Expand Up @@ -119,8 +119,7 @@ exports[`EuiContextMenuPanel updating items and content updates to items should
<button className=\\"euiContextMenuItem\\" type=\\"button\\" disabled={[undefined]} data-counter={0}>
<span className=\\"euiContextMenu__itemLayout\\">
<span className=\\"euiContextMenuItem__text\\">
Option
A
Option A
</span>
</span>
</button>
Expand All @@ -129,8 +128,7 @@ exports[`EuiContextMenuPanel updating items and content updates to items should
<button className=\\"euiContextMenuItem\\" type=\\"button\\" disabled={[undefined]} data-counter={1}>
<span className=\\"euiContextMenu__itemLayout\\">
<span className=\\"euiContextMenuItem__text\\">
Option
B
Option B
</span>
</span>
</button>
Expand All @@ -148,8 +146,7 @@ exports[`EuiContextMenuPanel updating items and content updates to items should
<button className=\\"euiContextMenuItem\\" type=\\"button\\" disabled={[undefined]} data-counter={0}>
<span className=\\"euiContextMenu__itemLayout\\">
<span className=\\"euiContextMenuItem__text\\">
Option
A
Option A
</span>
</span>
</button>
Expand All @@ -158,8 +155,7 @@ exports[`EuiContextMenuPanel updating items and content updates to items should
<button className=\\"euiContextMenuItem\\" type=\\"button\\" disabled={[undefined]} data-counter={1}>
<span className=\\"euiContextMenu__itemLayout\\">
<span className=\\"euiContextMenuItem__text\\">
Option
B
Option B
</span>
</span>
</button>
Expand Down Expand Up @@ -197,8 +193,7 @@ exports[`EuiContextMenuPanel updating items and content updates to items should
<button className=\\"euiContextMenuItem\\" type=\\"button\\" disabled={[undefined]} data-counter={0}>
<span className=\\"euiContextMenu__itemLayout\\">
<span className=\\"euiContextMenuItem__text\\">
Option
A
Option A
</span>
</span>
</button>
Expand All @@ -207,8 +202,7 @@ exports[`EuiContextMenuPanel updating items and content updates to items should
<button className=\\"euiContextMenuItem\\" type=\\"button\\" disabled={[undefined]} data-counter={1}>
<span className=\\"euiContextMenu__itemLayout\\">
<span className=\\"euiContextMenuItem__text\\">
Option
B
Option B
</span>
</span>
</button>
Expand All @@ -226,8 +220,7 @@ exports[`EuiContextMenuPanel updating items and content updates to items should
<button className=\\"euiContextMenuItem\\" type=\\"button\\" disabled={[undefined]} data-counter={2}>
<span className=\\"euiContextMenu__itemLayout\\">
<span className=\\"euiContextMenuItem__text\\">
Option
A
Option A
</span>
</span>
</button>
Expand All @@ -236,8 +229,7 @@ exports[`EuiContextMenuPanel updating items and content updates to items should
<button className=\\"euiContextMenuItem\\" type=\\"button\\" disabled={[undefined]} data-counter={3}>
<span className=\\"euiContextMenu__itemLayout\\">
<span className=\\"euiContextMenuItem__text\\">
Option
B
Option B
</span>
</span>
</button>
Expand Down
10 changes: 8 additions & 2 deletions src/components/context_menu/context_menu_panel.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -254,14 +254,20 @@ export class EuiContextMenuPanel extends Component {
return true;
}

// Check if any watched item properties changed by quick string comparison
// **
// this component should have either items or children,
// if there are items we can determine via `watchedItemProps` if we should update
// if there are children we can't know if they have changed so return true
// **

if (this.props.items != null) {
// Check if any watched item properties changed by quick string comparison
if(this.getWatchedPropsForItems(nextProps.items) !== this.getWatchedPropsForItems(this.props.items)) {
return true;
}
}

// if this panel was given any children, return true
// it's not possible (in any good way) to know if `children` has changed, assume they might have
if (this.props.children != null) {
return true;
}
Expand Down
144 changes: 56 additions & 88 deletions src/components/context_menu/context_menu_panel.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -368,112 +368,80 @@ describe('EuiContextMenuPanel', () => {
describe('updating items and content', () => {
describe('updates to items', () => {
it(`should not re-render if any items's watchedItemProps did not change`, () => {
return new Promise((resolve, reject) => {
try {
const getItem = (() => {
let counter = 0;
return key => {
return (
<EuiContextMenuItem key={key} data-counter={counter++}>
Option {key}
</EuiContextMenuItem>
)
}
})();

function makeItems() {
return [getItem('A'), getItem('B')];
}

// by not passing `watchedItemProps` no changes to items should cause a re-render
const component = mount(
<EuiContextMenuPanel
items={makeItems()}
/>
);
expect.assertions(2);

expect(component.debug()).toMatchSnapshot();
// by not passing `watchedItemProps` no changes to items should cause a re-render
const component = mount(
<EuiContextMenuPanel
items={[
<EuiContextMenuItem key="A" data-counter={0}>Option A</EuiContextMenuItem>,
<EuiContextMenuItem key="B" data-counter={1}>Option B</EuiContextMenuItem>,
]}
/>
);

component.setProps(
{ items: makeItems() },
() => {
expect(component.debug()).toMatchSnapshot();
}
);
expect(component.debug()).toMatchSnapshot();

resolve();
} catch(e) {
reject(e);
component.setProps(
{
items: [
<EuiContextMenuItem key="A" data-counter={2}>Option A</EuiContextMenuItem>,
<EuiContextMenuItem key="B" data-counter={3}>Option B</EuiContextMenuItem>
]
},
() => {
expect(component.debug()).toMatchSnapshot();
}
});
);
});

it(`should re-render if any items's watchedItemProps did change`, () => {
return new Promise((resolve, reject) => {
try {
const getItem = (() => {
let counter = 0;
return key => {
return (
<EuiContextMenuItem key={key} data-counter={counter++}>
Option {key}
</EuiContextMenuItem>
)
}
})();

function makeItems() {
return [getItem('A'), getItem('B')];
}

// by referencing the `data-counter` property in `watchedItemProps`
// changes to the items should be picked up and re-rendered
const component = mount(
<EuiContextMenuPanel
watchedItemProps={['data-counter']}
items={makeItems()}
/>
);
expect.assertions(2);

expect(component.debug()).toMatchSnapshot();
// by referencing the `data-counter` property in `watchedItemProps`
// changes to the items should be picked up and re-rendered
const component = mount(
<EuiContextMenuPanel
watchedItemProps={['data-counter']}
items={[
<EuiContextMenuItem key="A" data-counter={0}>Option A</EuiContextMenuItem>,
<EuiContextMenuItem key="B" data-counter={1}>Option B</EuiContextMenuItem>,
]}
/>
);

component.setProps(
{ items: makeItems() },
() => {
expect(component.debug()).toMatchSnapshot();
}
);
expect(component.debug()).toMatchSnapshot();

resolve();
} catch(e) {
reject(e);
component.setProps(
{
items: [
<EuiContextMenuItem key="A" data-counter={2}>Option A</EuiContextMenuItem>,
<EuiContextMenuItem key="B" data-counter={3}>Option B</EuiContextMenuItem>,
]
},
() => {
expect(component.debug()).toMatchSnapshot();
}
});
);
});

it(`should re-render at all times when children exists`, () => {
return new Promise((resolve, reject) => {
try {
const component = mount(
<EuiContextMenuPanel>
Hello World
</EuiContextMenuPanel>
);
expect.assertions(2);

expect(component.debug()).toMatchSnapshot();
const component = mount(
<EuiContextMenuPanel>
Hello World
</EuiContextMenuPanel>
);

component.setProps(
{ children: 'More Salutations' },
() => {
expect(component.debug()).toMatchSnapshot();
}
);
expect(component.debug()).toMatchSnapshot();

resolve();
} catch(e) {
reject(e);
component.setProps(
{ children: 'More Salutations' },
() => {
expect(component.debug()).toMatchSnapshot();
}
});
);
});
});
});
Expand Down

0 comments on commit 2362caf

Please sign in to comment.