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

[Lens] Make breadcrumbs look and feel like Visualize #44258

Merged
merged 3 commits into from
Sep 4, 2019
Merged

[Lens] Make breadcrumbs look and feel like Visualize #44258

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
24a98cd
Make Lens breadcrumbs match Visualize, so
chrisdavies Aug 28, 2019
1f12180
Test breadcrumb effect
chrisdavies Aug 30, 2019
c48fa23
Fix TypeScript errors
chrisdavies Sep 3, 2019
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
36 changes: 35 additions & 1 deletion x-pack/legacy/plugins/lens/public/app_plugin/app.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
*/

import React from 'react';
import { App } from './app';
import { App, setBreadcrumbs } from './app';
import { EditorFrameInstance } from '../types';

import { Chrome } from 'ui/chrome';
Expand Down Expand Up @@ -44,6 +44,8 @@ function makeDefaultArgs(): jest.Mocked<{
return ({
editorFrame: createMockFrame(),
chrome: {
addBasePath: (s: string) => `/testbasepath/${s}`,
breadcrumbs: { set: jest.fn() },
getUiSettingsClient() {
return {
get: jest.fn(type => {
Expand Down Expand Up @@ -114,6 +116,38 @@ describe('Lens App', () => {
`);
});

describe('setBreadcrumbs', () => {
it('contains only Visualize if no document is passed', () => {
const mockSet = jest.fn();
setBreadcrumbs({
addBasePath: (s: string) => `/testbasepath${s}`,
breadcrumbs: { set: mockSet },
});

expect(mockSet).toHaveBeenCalledWith([
{ text: 'Visualize', href: '/testbasepath/app/kibana#/visualize' },
]);
});

it('shows the document title if a document is passed', () => {
const mockSet = jest.fn();
setBreadcrumbs(
{
addBasePath: (s: string) => `/testbasepath${s}`,
breadcrumbs: { set: mockSet },
},
{
title: 'If you want it done, go. If not, send.',
}
);

expect(mockSet).toHaveBeenCalledWith([
{ text: 'Visualize', href: '/testbasepath/app/kibana#/visualize' },
{ text: 'If you want it done, go. If not, send.' },
]);
});
});
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see a test for the useEffect you added, which would be if the persisted title is changed, the breadcrumb should change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah... It's true. It's clumsy to test, and I felt like I was just testing mocks, mostly, but it's probably worth something basic at least.


describe('persistence', () => {
it('does not load a document if there is no document id', () => {
const args = makeDefaultArgs();
Expand Down
23 changes: 23 additions & 0 deletions x-pack/legacy/plugins/lens/public/app_plugin/app.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,24 @@ function isLocalStateDirty(
);
}

// Exported for testing purposes, sets the Kibana breadcrumbs.
export function setBreadcrumbs(
chrome: {
Copy link
Contributor

Choose a reason for hiding this comment

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

chrome should be well typed by {Chrome} from 'ui/chrome'

Copy link
Contributor Author

@chrisdavies chrisdavies Aug 30, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, but that pulls in a lot of baggage that we aren't using here, and requires a comprehensive mock of Chrome. This is kind of doing the I in SOLID in a way-- preferring narrow, specific interfaces to large, general ones.

Hm. But I could do a Pick<Chrome...>. So, good point!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Turns out it still requires a lot more mocking, at which point, so I think it's worth defining a narrower interface in the function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Last comment... I was only exporting the function for testing purposes in order to avoid the complexity of testing React effects. But Wylie asked me to test the effect, so I just got rid of the function and the point about Chrome is now moot!

addBasePath: (url: string) => string;
breadcrumbs: { set: (crumbs: Array<{ text: string; href?: string }>) => void };
},
persistedDoc?: { title: string }
) {
const rootCrumb = {
href: chrome.addBasePath(`/app/kibana#/visualize`),
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure whether we want to open that can of worms, but you can navigate back to the visualize listing with unsaved changes and lose them because we don't persist to the url.

Maps solves this by a browser prompt, either that or a modal would be nice
Screenshot 2019-08-30 at 09 38 33

The same thing would apply to navigating away from the page by clicking another app or hitting back, so probably out of scope for this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. I think it's unrelated, though. I'll make sure we have an issue for warning when you're about to lose changes.

text: i18n.translate('xpack.lens.breadcrumbsTitle', {
defaultMessage: 'Visualize',
}),
};

chrome.breadcrumbs.set(persistedDoc ? [rootCrumb, { text: persistedDoc.title }] : [rootCrumb]);
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't feel super strong about this, but in Visualize there is a "Create" crumb if the current is not saved:
Screenshot 2019-08-30 at 09 36 03

Feels like we should do the same thing

}

export function App({
editorFrame,
store,
Expand Down Expand Up @@ -87,6 +105,11 @@ export function App({

const lastKnownDocRef = useRef<Document | undefined>(undefined);

// Sync Kibana breadcrumbs any time we save the document
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: comment should be // Sync Kibana breadcrumbs any time we change the title of the saved document

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True.

useEffect(() => setBreadcrumbs(chrome, state.persistedDoc), [
state.persistedDoc && state.persistedDoc.title,
]);

useEffect(() => {
if (docId && (!state.persistedDoc || state.persistedDoc.id !== docId)) {
setState({ ...state, isLoading: true });
Expand Down