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

Add new page/route for notebooks create and rename #152

Merged
merged 22 commits into from
Feb 6, 2023
+99 −35
Merged

Add new page/route for notebooks create and rename #152

Show file tree
Hide file tree
Changes from 20 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
015d929
initial commit to get new page rendered for notebook create
derek-ho Nov 30, 2022
8a7b4d1
clean up and new file for create
derek-ho Nov 30, 2022
5b388fc
get breadcrumb working and page styling
derek-ho Nov 30, 2022
ad615c3
pushing work
derek-ho Dec 1, 2022
6152d33
get it mainly working
derek-ho Dec 1, 2022
7682fc9
fix existing test
derek-ho Dec 1, 2022
613496a
clean up code and include two trivial tests
derek-ho Dec 1, 2022
c9e610a
push work
derek-ho Dec 1, 2022
6dd53cd
remove clone for now
derek-ho Dec 1, 2022
28bc0bc
fix component
derek-ho Dec 1, 2022
c4ceca5
update a test
derek-ho Dec 5, 2022
6355fbe
add test for create
derek-ho Dec 5, 2022
fe62059
fix cypress test
derek-ho Dec 7, 2022
05f5a80
PR comment fixes
derek-ho Dec 9, 2022
b47634b
fix test
derek-ho Dec 12, 2022
7f89652
fix up
derek-ho Feb 2, 2023
d45370a
clean up
derek-ho Feb 2, 2023
70b6253
revert integration test changes
derek-ho Feb 2, 2023
d6e8f87
fix test
derek-ho Feb 2, 2023
da2769e
fix test
derek-ho Feb 2, 2023
cc0763c
PR comments
derek-ho Feb 6, 2023
e9eabb7
fix test
derek-ho Feb 6, 2023
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
23 changes: 13 additions & 10 deletions public/components/notebooks/components/__tests__/__snapshots__/note_table.test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,9 +132,10 @@ exports[`<NoteTable /> spec renders the component 1`] = `
<div
class="euiFlexItem"
>
<button
<a
class="euiButton euiButton--primary euiButton--fill"
type="button"
href="#/notebooks/create"
rel="noreferrer"
>
<span
class="euiButtonContent euiButton__content"
Expand All @@ -145,7 +146,7 @@ exports[`<NoteTable /> spec renders the component 1`] = `
Create notebook
</span>
</span>
</button>
</a>
</div>
</div>
</div>
Expand Down Expand Up @@ -1022,9 +1023,10 @@ exports[`<NoteTable /> spec renders the empty component 1`] = `
<div
class="euiFlexItem"
>
<button
<a
class="euiButton euiButton--primary euiButton--fill"
type="button"
href="#/notebooks/create"
rel="noreferrer"
>
<span
class="euiButtonContent euiButton__content"
Expand All @@ -1035,7 +1037,7 @@ exports[`<NoteTable /> spec renders the empty component 1`] = `
Create notebook
</span>
</span>
</button>
</a>
</div>
</div>
</div>
Expand Down Expand Up @@ -1080,10 +1082,11 @@ exports[`<NoteTable /> spec renders the empty component 1`] = `
<div
class="euiFlexItem euiFlexItem--flexGrowZero"
>
<button
class="euiButton euiButton--primary"
<a
class="euiButton euiButton--primary euiButton--fill"
data-test-subj="note-table-empty-state-create-notebook-button"
type="button"
href="#/notebooks/create"
rel="noreferrer"
>
<span
class="euiButtonContent euiButton__content"
Expand All @@ -1094,7 +1097,7 @@ exports[`<NoteTable /> spec renders the empty component 1`] = `
Create notebook
</span>
</span>
</button>
</a>
</div>
<div
class="euiFlexItem euiFlexItem--flexGrowZero"
Expand Down
51 changes: 49 additions & 2 deletions public/components/notebooks/components/__tests__/note_table.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,23 @@
* SPDX-License-Identifier: Apache-2.0
*/

import { fireEvent, render } from '@testing-library/react';
import { fireEvent, render, waitFor } from '@testing-library/react';
import { configure, mount, shallow } from 'enzyme';
import Adapter from 'enzyme-adapter-react-16';
import React from 'react';
import { NoteTable } from '../note_table';

jest.mock('react-router-dom', () => ({
useLocation: jest.fn().mockReturnValue({
pathname: '/notebooks',
search: '',
hash: '',
state: null,
key: '',
}),
useHistory: jest.fn()
}));

describe('<NoteTable /> spec', () => {
configure({ adapter: new Adapter() });

Expand Down Expand Up @@ -81,4 +92,40 @@ describe('<NoteTable /> spec', () => {
utils.getByText('Actions').click();
utils.getByText('Rename').click();
});
});

it('create notebook', async () => {
const fetchNotebooks = jest.fn();
const addSampleNotebooks = jest.fn();
const createNotebook = jest.fn();
const renameNotebook = jest.fn();
const cloneNotebook = jest.fn();
const deleteNotebook = jest.fn();
const setBreadcrumbs = jest.fn();
const setToast = jest.fn();
const notebooks = Array.from({ length: 5 }, (v, k) => ({
path: `path-${k}`,
id: `id-${k}`,
dateCreated: 'date-created',
dateModified: 'date-modified',
}));
const utils = render(
<NoteTable
loading={false}
fetchNotebooks={fetchNotebooks}
addSampleNotebooks={addSampleNotebooks}
notebooks={notebooks}
createNotebook={createNotebook}
renameNotebook={renameNotebook}
cloneNotebook={cloneNotebook}
deleteNotebook={deleteNotebook}
parentBreadcrumb={{ href: 'parent-href', text: 'parent-text' }}
setBreadcrumbs={setBreadcrumbs}
setToast={setToast}
/>
);
utils.getByText('Create notebook').click();
await waitFor(() => {
expect(global.window.location.href).toContain('/create')
});
});
});
42 changes: 22 additions & 20 deletions public/components/notebooks/components/main.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -308,26 +308,8 @@ export class Main extends React.Component<MainProps, MainState> {
/>
<Switch>
<Route
path="/notebooks/:id"
render={(props) => (
<Notebook
pplService={this.props.pplService}
openedNoteId={props.match.params.id}
DashboardContainerByValueRenderer={this.props.DashboardContainerByValueRenderer}
http={this.props.http}
parentBreadcrumb={this.props.parentBreadcrumb}
setBreadcrumbs={this.props.setBreadcrumbs}
renameNotebook={this.renameNotebook}
cloneNotebook={this.cloneNotebook}
deleteNotebook={this.deleteNotebook}
setToast={this.setToast}
location={this.props.location}
history={this.props.history}
/>
)}
/>
<Route
path="/notebooks"
exact
path={['/notebooks/create', '/notebooks']}
render={(props) => (
<ObservabilitySideBar>
<NoteTable
Expand All @@ -346,6 +328,26 @@ export class Main extends React.Component<MainProps, MainState> {
</ObservabilitySideBar>
)}
/>
<Route
exact
path="/notebooks/:id"
render={(props) => (
<Notebook
pplService={this.props.pplService}
openedNoteId={props.match.params.id}
DashboardContainerByValueRenderer={this.props.DashboardContainerByValueRenderer}
http={this.props.http}
parentBreadcrumb={this.props.parentBreadcrumb}
setBreadcrumbs={this.props.setBreadcrumbs}
renameNotebook={this.renameNotebook}
cloneNotebook={this.cloneNotebook}
deleteNotebook={this.deleteNotebook}
setToast={this.setToast}
location={this.props.location}
history={this.props.history}
/>
)}
/>
</Switch>
</>
</HashRouter>
Expand Down
20 changes: 16 additions & 4 deletions public/components/notebooks/components/note_table.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ import {
} from './helpers/modal_containers';
import { NotebookType } from './main';
import { pageStyles } from '../../../../common/constants/shared';
import { useHistory, useLocation } from 'react-router-dom';

interface NoteTableProps {
loading: boolean;
Expand All @@ -64,6 +65,8 @@ export function NoteTable(props: NoteTableProps) {
const [isActionsPopoverOpen, setIsActionsPopoverOpen] = useState(false);
const [selectedNotebooks, setSelectedNotebooks] = useState<NotebookType[]>([]);
const [searchQuery, setSearchQuery] = useState('');
const location = useLocation();
const history = useHistory();
const { notebooks, createNotebook, renameNotebook, cloneNotebook, deleteNotebook } = props;

useEffect(() => {
Expand All @@ -77,6 +80,13 @@ export function NoteTable(props: NoteTableProps) {
props.fetchNotebooks();
}, []);

useEffect(() => {
const url = window.location.hash.split('/')
if (url[url.length-1] === 'create') {
createNote();
}
}, [location]);

Comment on lines +83 to +89
Copy link
Member

Choose a reason for hiding this comment

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

Nice 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

@derek-ho FYI future code changes (don't address for this PR)

Array.at(-1) vs arr[arr.length - 1]
The usual practice is to access length and calculate the index from that — for example, array[array.length - 1]. The at() method allows relative indexing, so this can be shortened to array.at(-1).

const closeModal = () => {
setIsModalVisible(false);
};
Expand Down Expand Up @@ -114,7 +124,10 @@ export function NoteTable(props: NoteTableProps) {
setModalLayout(
getCustomModal(
onCreate,
closeModal,
() => {
closeModal()
history.push('/notebooks')
Copy link
Member

Choose a reason for hiding this comment

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

Should this be the /notebooks or the create workflow originator page?

Copy link
Collaborator Author

@derek-ho derek-ho Feb 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Good catch - I missed this as I was mainly testing it from the notebooks page itself, not the connection from other pages. Fixed with history.goback() in cc0763c

},
'Name',
'Create notebook',
'Cancel',
Expand Down Expand Up @@ -306,7 +319,7 @@ export function NoteTable(props: NoteTableProps) {
</EuiPopover>
</EuiFlexItem>
<EuiFlexItem>
<EuiButton fill onClick={() => createNote()}>
<EuiButton fill href="#/notebooks/create">
Create notebook
</EuiButton>
</EuiFlexItem>
Expand Down Expand Up @@ -367,10 +380,9 @@ export function NoteTable(props: NoteTableProps) {
<EuiSpacer size="m" />
<EuiFlexGroup justifyContent="center">
<EuiFlexItem grow={false}>
<EuiButton
<EuiButton fill href="#/notebooks/create"
Copy link
Member

Choose a reason for hiding this comment

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

Let's revert this fill as it was previously. Usually, a page should only have 1 primary fill button to implicitly tell the user that this is the main button to click on.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I see... Fixed in cc0763c

data-test-subj="note-table-empty-state-create-notebook-button"
fullWidth={false}
onClick={() => createNote()}
>
Create notebook
</EuiButton>
Expand Down