Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix props validation for Breadcrumb #4666

Merged
merged 4 commits into from
Apr 25, 2018
Merged

Fix props validation for Breadcrumb #4666

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2c6d4cb
Fix props validation for Breadcrumb and add unit tests for default props
ThomasMichon Apr 24, 2018
b714b79
Update snapshot output
ThomasMichon Apr 24, 2018
6308b5d
Update change output
ThomasMichon Apr 24, 2018
45c7a49
Fix Breadcrumb validation
ThomasMichon Apr 25, 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
11 changes: 11 additions & 0 deletions common/changes/office-ui-fabric-react/breadcrumb-validation_2018-04-24-20-18.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
{
"changes": [
{
"packageName": "office-ui-fabric-react",
"comment": "Fix props validation for Breadcrumb",
"type": "patch"
}
],
"packageName": "office-ui-fabric-react",
"email": "tmichon@microsoft.com"
}
6 changes: 4 additions & 2 deletions packages/office-ui-fabric-react/src/components/Breadcrumb/Breadcrumb.base.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,8 +203,10 @@ export class Breadcrumb extends BaseComponent<IBreadcrumbProps, any> {
* @param props Props to validate
*/
private _validateProps(props: IBreadcrumbProps): void {
const { items, overflowIndex } = props;
if (overflowIndex! < 0 || overflowIndex! > items.length - 1) {
const { maxDisplayedItems, overflowIndex, items } = props;
if (overflowIndex! < 0 ||
maxDisplayedItems! > 1 && overflowIndex! > maxDisplayedItems! - 1 ||
Copy link
Member

@JasonGore JasonGore Apr 25, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Should overflowIndex be compared against maxDisplayedItems when maxDisplayedItems is 1?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this is enough sanity checking for the moment. The component doesn't actually break if you specify values out-of-bounds.

items.length > 0 && overflowIndex! > items.length - 1) {
throw new Error('Breadcrumb: overflowIndex out of range');
}
}
Expand Down
104 changes: 61 additions & 43 deletions packages/office-ui-fabric-react/src/components/Breadcrumb/Breadcrumb.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,17 @@ import * as renderer from 'react-test-renderer';
import { Breadcrumb, IBreadcrumbItem } from './index';

describe('Breadcrumb', () => {
it('renders breadcumb correctly', () => {
it('renders empty breadcrumb', () => {
const component = renderer.create(
<Breadcrumb items={ [] } />
);

const tree = component.toJSON();

expect(tree).toMatchSnapshot();
});

describe('basic rendering', () => {
const items: IBreadcrumbItem[] = [
{ text: 'TestText1', key: 'TestKey1' },
{ text: 'TestText2', key: 'TestKey2' },
Expand All @@ -16,48 +26,56 @@ describe('Breadcrumb', () => {

const divider = () => <span>*</span>;

let component = renderer.create(
<Breadcrumb
items={ items }
/>
);

let tree = component.toJSON();
expect(tree).toMatchSnapshot();

// With overflow
component = renderer.create(
<Breadcrumb
items={ items }
maxDisplayedItems={ 2 }
/>
);

tree = component.toJSON();
expect(tree).toMatchSnapshot();

// With custom divider
component = renderer.create(
<Breadcrumb
items={ items }
dividerAs={ divider }
/>
);

tree = component.toJSON();
expect(tree).toMatchSnapshot();

// With overflow and overflowIndex
component = renderer.create(
<Breadcrumb
items={ items }
maxDisplayedItems={ 2 }
overflowIndex={ 1 }
/>
);

tree = component.toJSON();
expect(tree).toMatchSnapshot();
it('renders breadcumb correctly 1', () => {
const component = renderer.create(
<Breadcrumb
items={ items }
/>
);

const tree = component.toJSON();
expect(tree).toMatchSnapshot();
});

it('renders breadcumb correctly 2', () => {
// With overflow
const component = renderer.create(
<Breadcrumb
items={ items }
maxDisplayedItems={ 2 }
/>
);

const tree = component.toJSON();
expect(tree).toMatchSnapshot();
});

it('renders breadcumb correctly 3', () => {
// With custom divider
const component = renderer.create(
<Breadcrumb
items={ items }
dividerAs={ divider }
/>
);

const tree = component.toJSON();
expect(tree).toMatchSnapshot();
});

it('renders breadcumb correctly 4', () => {
// With overflow and overflowIndex
const component = renderer.create(
<Breadcrumb
items={ items }
maxDisplayedItems={ 2 }
overflowIndex={ 1 }
/>
);

const tree = component.toJSON();
expect(tree).toMatchSnapshot();
});
});

it('can call the callback when an item is clicked', () => {
Expand Down
57 changes: 49 additions & 8 deletions ...s/office-ui-fabric-react/src/components/Breadcrumb/__snapshots__/Breadcrumb.test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`Breadcrumb renders breadcumb correctly 1`] = `
exports[`Breadcrumb basic rendering renders breadcumb correctly 1 1`] = `
<div
className=
ms-ResizeGroup
Expand All @@ -26,7 +26,7 @@ exports[`Breadcrumb renders breadcumb correctly 1`] = `
aria-describedby={undefined}
aria-labelledby={undefined}
className="ms-FocusZone"
data-focuszone-id="FocusZone0"
data-focuszone-id="FocusZone1"
onFocus={[Function]}
onKeyDown={[Function]}
onMouseDownCapture={[Function]}
Expand Down Expand Up @@ -144,7 +144,7 @@ exports[`Breadcrumb renders breadcumb correctly 1`] = `
</div>
`;

exports[`Breadcrumb renders breadcumb correctly 2`] = `
exports[`Breadcrumb basic rendering renders breadcumb correctly 2 1`] = `
<div
className=
ms-ResizeGroup
Expand All @@ -170,7 +170,7 @@ exports[`Breadcrumb renders breadcumb correctly 2`] = `
aria-describedby={undefined}
aria-labelledby={undefined}
className="ms-FocusZone"
data-focuszone-id="FocusZone5"
data-focuszone-id="FocusZone6"
onFocus={[Function]}
onKeyDown={[Function]}
onMouseDownCapture={[Function]}
Expand Down Expand Up @@ -355,7 +355,7 @@ exports[`Breadcrumb renders breadcumb correctly 2`] = `
</div>
`;

exports[`Breadcrumb renders breadcumb correctly 3`] = `
exports[`Breadcrumb basic rendering renders breadcumb correctly 3 1`] = `
<div
className=
ms-ResizeGroup
Expand All @@ -381,7 +381,7 @@ exports[`Breadcrumb renders breadcumb correctly 3`] = `
aria-describedby={undefined}
aria-labelledby={undefined}
className="ms-FocusZone"
data-focuszone-id="FocusZone11"
data-focuszone-id="FocusZone12"
onFocus={[Function]}
onKeyDown={[Function]}
onMouseDownCapture={[Function]}
Expand Down Expand Up @@ -478,7 +478,7 @@ exports[`Breadcrumb renders breadcumb correctly 3`] = `
</div>
`;

exports[`Breadcrumb renders breadcumb correctly 4`] = `
exports[`Breadcrumb basic rendering renders breadcumb correctly 4 1`] = `
<div
className=
ms-ResizeGroup
Expand All @@ -504,7 +504,7 @@ exports[`Breadcrumb renders breadcumb correctly 4`] = `
aria-describedby={undefined}
aria-labelledby={undefined}
className="ms-FocusZone"
data-focuszone-id="FocusZone16"
data-focuszone-id="FocusZone17"
onFocus={[Function]}
onKeyDown={[Function]}
onMouseDownCapture={[Function]}
Expand Down Expand Up @@ -688,3 +688,44 @@ exports[`Breadcrumb renders breadcumb correctly 4`] = `
</div>
</div>
`;

exports[`Breadcrumb renders empty breadcrumb 1`] = `
<div
className=
ms-ResizeGroup
{
display: block;
position: relative;
}
>
<div
style={
Object {
"position": "fixed",
"visibility": "hidden",
}
}
>
<div
aria-label={undefined}
className="ms-Breadcrumb"
role="navigation"
>
<div
aria-describedby={undefined}
aria-labelledby={undefined}
className="ms-FocusZone"
data-focuszone-id="FocusZone0"
onFocus={[Function]}
onKeyDown={[Function]}
onMouseDownCapture={[Function]}
role="presentation"
>
<ol
className="ms-Breadcrumb-list"
/>
</div>
</div>
</div>
</div>
`;