<EnvBuildStatus /> component #377

gabalafou · 2024-03-05T22:43:11Z

Follow up, clean up to #367.

Description

This pull request:

Refactors out some of the code in the EnvBuild component into EnvBuildStatus
Renames the export of BuildDropdown.tsx to match the file name
Some code clean up for clarity

Pull request checklist

Did you test this change locally?
Did you update the documentation (if required)?
Did you add/update relevant tests for this change (if required)?

Additional information

gabalafou · 2024-03-05T22:43:40Z

Marking this as draft until I update Storybook

gabalafou

Done self-reviewing. I decided it didn't make sense to create a separate Storybook story for the EnvBuildStatus component. I think the EnvBuilds story is sufficient.

src/features/metadata/components/EnvBuilds.tsx

src/utils/helpers/buildMapper.ts

test/helpers/BuildMapper.test.tsx

gabalafou · 2024-03-14T22:19:33Z

test/metadata/BuildDropdown.test.tsx

  });

  it("should display builds in the dropdown", async () => {
+    // This is what the dropdown should display for the build
+    const dropdownOptionName = buildDatetimeStatus(


Feels a little funky to me to call a function here rather than to just hard-code the string, perhaps I should go back and update this

Well, if your hesitation is that BuildDropdown uses buildDatetimeStatus to create the string, remember that you're already testing buildDatetimeStatus elsewhere. So I think this is fine, this test is really checking that the correct text appears in the element, not that buildDatetimeStatus makes the right string. Anyway, if you'd prefer this test looks different I'll defer to you, but IMO this is fine.

Good point, thanks!

src/features/metadata/components/BuildDropdown.tsx

peytondmurray

Thanks for this, this cleans things up nicely. The one main comment I had was the question about combining the BUILDING and QUEUED statuses into one label shown to the user, but otherwise this LGTM 🚢

src/utils/helpers/buildMapper.ts

peytondmurray · 2024-03-23T18:13:47Z

src/features/metadata/components/EnvBuildStatus.tsx

+  );
+};
+
+export interface IEnvBuildStatusProps {


What's the usual protocol around exporting these one-off interfaces? I know some projects do so and others don't.

Oh this isn't needed. It was needed by Storybook when I was importing this component into Storybook, but then I decided not to create a story for this component.

Suggested change

export interface IEnvBuildStatusProps {

interface IEnvBuildStatusProps {

peytondmurray · 2024-03-23T18:22:50Z

src/features/metadata/components/EnvBuildStatus.tsx

+}
+
+export const EnvBuildStatus = ({ build }: IEnvBuildStatusProps) => {
+  const logArtifact: Artifact | never = artifactList(build.id, ["LOGS"])[0];


I checked it out and this seems good, but I can't get over how weird this looks because indexing outside an array's length returns an undefined:

const foo = [1, 2, 3] foo[8] // Returns undefined

But from what I can tell typescript infers the type for [] to be never[] which is just strange. 🤔

No you're right. My type annotation was wrong. Fixed.

peytondmurray · 2024-03-23T18:31:24Z

test/metadata/BuildDropdown.test.tsx

  });

  it("should display builds in the dropdown", async () => {
+    // This is what the dropdown should display for the build
+    const dropdownOptionName = buildDatetimeStatus(


Well, if your hesitation is that BuildDropdown uses buildDatetimeStatus to create the string, remember that you're already testing buildDatetimeStatus elsewhere. So I think this is fine, this test is really checking that the correct text appears in the element, not that buildDatetimeStatus makes the right string. Anyway, if you'd prefer this test looks different I'll defer to you, but IMO this is fine.

peytondmurray · 2024-03-23T18:42:11Z

src/utils/helpers/buildMapper.ts

+  let duration = 0;
+  if (ended_on && scheduled_on) {
+    const startTime = new Date(scheduled_on);
+    const endTime = new Date(ended_on);
+    duration = (endTime.valueOf() - startTime.valueOf()) / 60000;
+    duration = Math.round(duration);
+  }


I guess you really only need to do this if status === "COMPLETED", so this can be moved inside the branch below.

peytondmurray · 2024-03-23T18:52:16Z

src/utils/helpers/buildMapper.ts

-  const BUILD_STATUS = ["QUEUED"];
-  return BUILD_STATUS.includes(status);
-};
+const isCompleted = ({


Maybe this is just a matter of preference but based just on this function's name I'd expect this to return a bool.

One other thing: this is only being called in buildStatus below, do we need to break this out into a separate function? You're already checking the value of status there, so this would be a bit shorter if you weren't checking it again here.

Good point, I'm fine with that, I'll update in a subsequent commit

peytondmurray · 2024-03-23T18:52:34Z

src/utils/helpers/buildMapper.ts

-        status: isCompleted(status, duration),
-        status_info
-      };
+export const buildStatus = ({


Do we need to combine the BUILDING and QUEUED statuses? It feels like it would be useful to have these distinct states represented distinctly in the UI.

Your suggestion makes sense but I'm a little afraid of it breaking something.

I also thought that maybe it had been a product decision to collapse those statuses for the end user. But that's not stated in the PR where it was introduced.

I'll ask about it.

Update. I asked @kcpevey on Slack and she said it looks like a mistake, so I've changed it and we'll see if anybody complains or it breaks anything.

gabalafou

Thanks for the thorough review @peytondmurray!

gabalafou · 2024-03-26T16:16:13Z

src/features/metadata/components/EnvBuildStatus.tsx

+  );
+};
+
+export interface IEnvBuildStatusProps {


Oh this isn't needed. It was needed by Storybook when I was importing this component into Storybook, but then I decided not to create a story for this component.

Suggested change

export interface IEnvBuildStatusProps {

interface IEnvBuildStatusProps {

gabalafou · 2024-03-26T16:18:41Z

src/utils/helpers/buildMapper.ts

-  const BUILD_STATUS = ["QUEUED"];
-  return BUILD_STATUS.includes(status);
-};
+const isCompleted = ({


Good point, I'm fine with that, I'll update in a subsequent commit

gabalafou · 2024-03-26T16:22:18Z

test/metadata/BuildDropdown.test.tsx

  });

  it("should display builds in the dropdown", async () => {
+    // This is what the dropdown should display for the build
+    const dropdownOptionName = buildDatetimeStatus(


Good point, thanks!

gabalafou · 2024-03-26T16:28:29Z

src/utils/helpers/buildMapper.ts

-        status: isCompleted(status, duration),
-        status_info
-      };
+export const buildStatus = ({


Your suggestion makes sense but I'm a little afraid of it breaking something.

I also thought that maybe it had been a product decision to collapse those statuses for the end user. But that's not stated in the PR where it was introduced.

I'll ask about it.

gabalafou · 2024-03-26T16:49:18Z

src/features/metadata/components/EnvBuildStatus.tsx

+}
+
+export const EnvBuildStatus = ({ build }: IEnvBuildStatusProps) => {
+  const logArtifact: Artifact | never = artifactList(build.id, ["LOGS"])[0];


No you're right. My type annotation was wrong. Fixed.

gabalafou · 2024-03-26T19:51:56Z

Tests pass, runs locally, looks good in Storybook. Merging in now.

gabalafou added 3 commits March 5, 2024 17:38

Add log link next to status message

7d72003

lint

03b4c64

code cleanup

fde86ae

gabalafou requested a review from peytondmurray March 5, 2024 22:43

gabalafou marked this pull request as draft March 5, 2024 22:43

gabalafou added 3 commits March 5, 2024 17:48

lint

7caa80f

refactor buildMapper

30d7ffa

more refaction

c03078f

gabalafou marked this pull request as ready for review March 14, 2024 21:45

better type annotation

8847ffb

gabalafou commented Mar 14, 2024

View reviewed changes

peytondmurray approved these changes Mar 23, 2024

View reviewed changes

gabalafou added 2 commits March 26, 2024 11:42

PR review

ed12797

fix type annotation

3d44501

gabalafou commented Mar 26, 2024

View reviewed changes

gabalafou merged commit 7e0fa2a into conda-incubator:main Mar 26, 2024
4 checks passed

gabalafou deleted the env-build-status-component branch March 26, 2024 19:52

gabalafou added a commit to gabalafou/conda-store-ui that referenced this pull request Jul 9, 2024

<EnvBuildStatus /> component (conda-incubator#377)

1b95715

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

<EnvBuildStatus /> component #377

<EnvBuildStatus /> component #377

gabalafou commented Mar 5, 2024 •

edited

Loading

gabalafou commented Mar 5, 2024

gabalafou left a comment

gabalafou Mar 14, 2024

peytondmurray Mar 23, 2024

gabalafou Mar 26, 2024

peytondmurray left a comment

peytondmurray Mar 23, 2024

gabalafou Mar 26, 2024

peytondmurray Mar 23, 2024

gabalafou Mar 26, 2024

peytondmurray Mar 23, 2024

peytondmurray Mar 23, 2024

peytondmurray Mar 23, 2024

gabalafou Mar 26, 2024

peytondmurray Mar 23, 2024

gabalafou Mar 26, 2024

gabalafou Mar 26, 2024

gabalafou left a comment

gabalafou Mar 26, 2024

gabalafou Mar 26, 2024

gabalafou Mar 26, 2024

gabalafou Mar 26, 2024

gabalafou Mar 26, 2024

gabalafou commented Mar 26, 2024

	export interface IEnvBuildStatusProps {
	interface IEnvBuildStatusProps {

<EnvBuildStatus /> component #377

<EnvBuildStatus /> component #377

Conversation

gabalafou commented Mar 5, 2024 • edited Loading

Description

Pull request checklist

Additional information

gabalafou commented Mar 5, 2024

gabalafou left a 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

peytondmurray left a 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

gabalafou left a 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

gabalafou commented Mar 26, 2024

gabalafou commented Mar 5, 2024 •

edited

Loading