refactor(ui): update machine header to support tag forms #3661

huwshimi · 2022-03-07T01:39:56Z

Done

Separate the main machine/pool/tag view and header so that it can support tag header forms.

QA

MAAS deployment

To run this branch you will need access to one of the following MAAS deployments:

Bolla
A development MAAS
A local snap MAAS (this will not usually have machines)

Running the branch

You can run this branch by:

Serving with dotrun
Building in a development MAAS

QA steps

Go to the machine view and check that you can open header forms.
Click on the pools tab and check that clicking on the add pool button opens the form.
Click on the tags tab and check that clicking on the add tag button opens the placeholder form in the header.

Fixes

Fixes: canonical-web-and-design/app-tribe#741.

huwshimi · 2022-03-07T01:43:57Z

ui/src/app/pools/views/PoolList/PoolList.test.tsx

@@ -0,0 +1,245 @@
+import { mount } from "enzyme";


This file hasn't changed, just renamed from ui/src/app/pools/views/Pools.test.tsx

huwshimi · 2022-03-07T01:44:06Z

ui/src/app/pools/views/PoolList/PoolList.tsx

@@ -0,0 +1,207 @@
+import { useEffect, useState } from "react";


This file hasn't been modified, just renamed from "ui/src/app/pools/views/Pools.tsx"

petermakowski · 2022-03-07T14:11:26Z

QA notes

Now the subheader (number of machines) disappears if you go to a different tab. This breaks the view hierarchy which is confusing (switching to a different tab should ideally only modify content that's within that tab). This is also not consistent with how we handle this pattern in other views (e.g. KVM).

This PR

Before

KVM

petermakowski · 2022-03-07T14:31:11Z

ui/src/app/base/components/node/MachinesHeader/MachinesHeader.test.tsx

@@ -0,0 +1,72 @@
+import { mount } from "enzyme";


Could you use the testing library please 🙏

I think we should probably have a rule that if you're touching a file you shouldn't have to update to RTL otherwise a lot of PRs are going to get very large.

petermakowski · 2022-03-07T14:39:51Z

ui/src/app/tags/components/TagsHeader/TagsHeader.test.tsx

+      </MemoryRouter>
+    </Provider>
+  );
+  expect(screen.getByText("Add tag form")).toBeInTheDocument();


Suggested change

expect(screen.getByText("Add tag form")).toBeInTheDocument();

expect(screen.getByRole("form", { name: "Add tag" })).toBeInTheDocument();

Could you use the appropriate role. This can easily get missed when you open another PR with the full form implementation.

I think it's OK, that text isn't going to stay...

I've added a todo here.

petermakowski · 2022-03-07T14:42:02Z

ui/src/app/pools/components/TagHeaderForms/TagHeaderForms.tsx

+        <>
+          Add tag form{" "}
+          <Button onClick={() => setHeaderContent(null)}>Close</Button>
+        </>


Suggested change

<>

Add tag form{" "}

<Button onClick={() => setHeaderContent(null)}>Close</Button>

</>

<form aria-label="Add tag">

<Button onClick={() => setHeaderContent(null)}>Close</Button>

</form>

petermakowski · 2022-03-07T14:44:56Z

ui/src/app/pools/components/TagHeaderForms/TagHeaderForms.test.tsx

+      setHeaderContent={jest.fn()}
+    />
+  );
+  expect(screen.getByText("Add tag form")).toBeInTheDocument();


Could you use the form role instead of simple text matching please.

This is just a placeholder until the form is implemented.

I've added a todo here.

petermakowski · 2022-03-07T14:55:27Z

ui/src/app/tags/components/TagsHeader/TagsHeader.tsx

+export const TagsHeader = ({
+  headerContent,
+  setHeaderContent,
+}: Props): JSX.Element => {
+  return (
+    <MachinesHeader
+      buttons={[
+        <Button
+          appearance="positive"
+          onClick={() => setHeaderContent({ view: TagHeaderViews.AddTag })}
+        >
+          Create new tag
+        </Button>,
+      ]}


This relates to my other comment about view hierarchy.

I think that instead of importing a parent element into a child it would make more sense to keep MachineHeader above and in a single place (views/Machines), like it was before.

The problem we have is that other than the title and the tabs we treat the machines/tags/pools as isolated parts of the site (there's no shared state etc.) so we're having to work around that by watching for URL changes and resetting things like the filter states.

As we're now adding forms for the tags we're now conflating things further. The aim of this PR is separating the concerns of each view (machines/tags/pools) so that they no longer have a single header and form state so each section can implement things like forms in isolation.

huwshimi · 2022-03-07T22:31:26Z

QA notes

Now the subheader (number of machines) disappears if you go to a different tab. This breaks the view hierarchy which is confusing (switching to a different tab should ideally only modify content that's within that tab). This is also not consistent with how we handle this pattern in other views (e.g. KVM).

Oh yeah that's to match the design:

Separate the main machine/pool/tag view and header so that it can support tag header forms. Fixes: canonical/app-tribe#741.

huwshimi added Review: Code needed Review: QA needed labels Mar 7, 2022

huwshimi commented Mar 7, 2022

View reviewed changes

petermakowski added Review: QA -1 and removed Review: QA needed labels Mar 7, 2022

petermakowski reviewed Mar 7, 2022

View reviewed changes

petermakowski added Review: Code -1 and removed Review: Code needed labels Mar 7, 2022

huwshimi added 2 commits March 8, 2022 09:32

refactor(ui): update machine header to support tag forms

6520bfa

Separate the main machine/pool/tag view and header so that it can support tag header forms. Fixes: canonical/app-tribe#741.

Add todos.

9742e22

huwshimi force-pushed the tags-machines-header branch from 9f43666 to 9742e22 Compare March 8, 2022 00:21

petermakowski added Review: Code +1 Review: QA +1 and removed Review: Code -1 Review: QA -1 labels Mar 8, 2022

petermakowski approved these changes Mar 8, 2022

View reviewed changes

huwshimi merged commit 6a28de7 into canonical:master Mar 8, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(ui): update machine header to support tag forms #3661

refactor(ui): update machine header to support tag forms #3661

huwshimi commented Mar 7, 2022

huwshimi Mar 7, 2022

huwshimi Mar 7, 2022

petermakowski commented Mar 7, 2022

petermakowski Mar 7, 2022

huwshimi Mar 7, 2022

petermakowski Mar 7, 2022

petermakowski Mar 7, 2022

huwshimi Mar 7, 2022

huwshimi Mar 8, 2022

petermakowski Mar 7, 2022

huwshimi Mar 7, 2022

petermakowski Mar 7, 2022

huwshimi Mar 7, 2022

huwshimi Mar 8, 2022

petermakowski Mar 8, 2022

petermakowski Mar 7, 2022

huwshimi Mar 7, 2022

huwshimi commented Mar 7, 2022 •

edited

Loading

QA notes

		@@ -0,0 +1,207 @@
		import { useEffect, useState } from "react";

	expect(screen.getByText("Add tag form")).toBeInTheDocument();
	expect(screen.getByRole("form", { name: "Add tag" })).toBeInTheDocument();

refactor(ui): update machine header to support tag forms #3661

refactor(ui): update machine header to support tag forms #3661

Conversation

huwshimi commented Mar 7, 2022

Done

QA

MAAS deployment

Running the branch

QA steps

Fixes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petermakowski commented Mar 7, 2022

QA notes

This PR

Before

KVM

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huwshimi commented Mar 7, 2022 • edited Loading

QA notes

huwshimi commented Mar 7, 2022 •

edited

Loading