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.

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

ui: move jobs pages to cluster-ui #82909

Merged
merged 1 commit into from
Jun 23, 2022

Conversation

ericharmeling
Copy link
Contributor

@ericharmeling ericharmeling commented Jun 14, 2022

This PR moves the Jobs page and the Jobs Details page to the cluster-ui package.

Related to #71324.

Release note: None

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@ericharmeling ericharmeling force-pushed the jobs-page-cluster-ui branch from 4e890b2 to cdffd4a Compare June 14, 2022 21:37
@ericharmeling ericharmeling requested a review from a team June 14, 2022 21:40
@ericharmeling ericharmeling force-pushed the jobs-page-cluster-ui branch 3 times, most recently from 7f52829 to e5e72d0 Compare June 16, 2022 12:58
Copy link
Contributor

@maryliag maryliag left a comment

Choose a reason for hiding this comment

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

Can you add a loom/video showing that is working normally on db console (you can show running locally)

Reviewed 25 of 31 files at r1.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @ericharmeling and @maryliag)


-- commits line 2 at r1:
We use the issue number from github, not jira here, so it gets properly connected, so you can add:

Related to #71324


pkg/ui/workspaces/cluster-ui/src/jobs/jobDetailsPage/jobDetails.tsx line 23 at r1 (raw file):

import { SummaryCard } from "src/summaryCard";
import { TimestampToMoment } from "src/util";
import { DATE_FORMAT } from "src/util/format";

can you confirm the timestamp used here? We're trying to use 24h UTC for all pages, so it will be good to use DATE_FORMAT_24_UTC, but make sure it's indeed in UTC
same thing for all usages of this format on this PR


pkg/ui/workspaces/cluster-ui/src/jobs/jobDetailsPage/jobDetails.tsx line 30 at r1 (raw file):

import { commonStyles } from "src/common";
import summaryCardstyles from "src/summaryCard/summaryCard.module.scss";

nit: summaryCardStyles


pkg/ui/workspaces/cluster-ui/src/jobs/jobs.module.scss line 59 at r1 (raw file):

  &__duration {
    font-family: SourceSansPro-Regular;
    font-size: 12px;

for fonts use the typography constants:
font-family: $font-family--base
font-size: $font-size--small

(for all cases below too)


pkg/ui/workspaces/cluster-ui/src/jobs/util/duration.tsx line 19 at r1 (raw file):

type Job = cockroach.server.serverpb.IJobResponse;

export const formatDuration = (d: moment.Duration) =>

I can see you copied this function to here, was it deleted from the original location?


pkg/ui/workspaces/cluster-ui/src/jobs/util/progressBar.tsx line 52 at r1 (raw file):

          strokeWidth={this.props.lineWidth}
          trailWidth={this.props.lineWidth}
          strokeColor="#0055ff"

can we keep using the value from design-token here?


pkg/ui/workspaces/cluster-ui/src/test-utils/tooltip.ts line 13 at r1 (raw file):

import { expect } from "chai";

export const expectPopperTooltipActivated = () =>

seems like this function is not being used anywhere, so the file itself can be deleted


pkg/ui/workspaces/db-console/src/views/jobs/jobTable.tsx line 289 at r1 (raw file):

              href={jobTable}
              target="_blank"
              onClick={this.redirectToLearnMore}

doesn't seem like the redirectToLearnMore is being used anywhere on this file anymore, so you can delete the function itself

@ericharmeling ericharmeling force-pushed the jobs-page-cluster-ui branch from e5e72d0 to 99ba1fe Compare June 21, 2022 15:56
Copy link
Contributor Author

@ericharmeling ericharmeling left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @maryliag)


-- commits line 2 at r1:

Previously, maryliag (Marylia Gutierrez) wrote…

We use the issue number from github, not jira here, so it gets properly connected, so you can add:

Related to #71324

Done.


pkg/ui/workspaces/cluster-ui/src/jobs/jobDetailsPage/jobDetails.tsx line 23 at r1 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

can you confirm the timestamp used here? We're trying to use 24h UTC for all pages, so it will be good to use DATE_FORMAT_24_UTC, but make sure it's indeed in UTC
same thing for all usages of this format on this PR

Done.


pkg/ui/workspaces/cluster-ui/src/jobs/jobDetailsPage/jobDetails.tsx line 30 at r1 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

nit: summaryCardStyles

Fixed.


pkg/ui/workspaces/cluster-ui/src/jobs/jobs.module.scss line 59 at r1 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

for fonts use the typography constants:
font-family: $font-family--base
font-size: $font-size--small

(for all cases below too)

Done.


pkg/ui/workspaces/cluster-ui/src/jobs/util/duration.tsx line 19 at r1 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

I can see you copied this function to here, was it deleted from the original location?

Yes. It was originally defined in and exported from index.tsx (which I've renamed to jobsPage.tsx), but never called in that file. I think it makes more sense to have it defined in duration.tsx, where it is actually used.


pkg/ui/workspaces/cluster-ui/src/jobs/util/progressBar.tsx line 52 at r1 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

can we keep using the value from design-token here?

I was having trouble getting this component to build with the design-token import, but after rebasing... it looks like it works now! 🤷‍♂️


pkg/ui/workspaces/cluster-ui/src/test-utils/tooltip.ts line 13 at r1 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

seems like this function is not being used anywhere, so the file itself can be deleted

Done.

FWIW, this function/file was introduced in #72291 in order to test a tooltip pop-up message for retrying jobs. I was having trouble getting that test to pass in my cluster-ui build of the jobs page with the expectPopperTooltipActivated call, and I honestly wasn't sure what calling expectPopperTooltipActivated before userEvent.hover was even doing for the test, so I simplified the test by removing expectPopperTooltipActivated from the test altogether (https://github.com/cockroachdb/cockroach/pull/82909/files#diff-e270da42dd3934f5fca2b6f42d02e9bbd3404471f75e5385f39cdffd9999d9dcR115), and it seems to be passing just fine now and testing on the same condition.


pkg/ui/workspaces/db-console/src/views/jobs/jobTable.tsx line 289 at r1 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

doesn't seem like the redirectToLearnMore is being used anywhere on this file anymore, so you can delete the function itself

Done.

@ericharmeling
Copy link
Contributor Author

Here's the a video of the jobs pages on the DB console for cockroach demo:

Screen.Recording.2022-06-21.at.11.53.53.AM.mov

@ericharmeling ericharmeling requested a review from maryliag June 21, 2022 16:15
Copy link
Contributor

@maryliag maryliag left a comment

Choose a reason for hiding this comment

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

Awesome!! :lgtm:

Reviewed 2 of 31 files at r1, 6 of 6 files at r2, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @ericharmeling)

@ericharmeling ericharmeling force-pushed the jobs-page-cluster-ui branch 2 times, most recently from a123081 to fdb0d24 Compare June 22, 2022 00:09
@ericharmeling
Copy link
Contributor Author

ericharmeling commented Jun 22, 2022

can we keep using the value from design-token here?

I was having trouble getting this component to build with the design-token import, but after rebasing... it looks like it works now! 🤷‍♂️

I was wrong about the design-tokens import causing a build failure. It's causing a test failure. It looks like there is a syntax incompatibility with the design-tokens export?

Summary of all failing tests
FAIL src/jobs/jobsPage/jobsPage.spec.tsx
  ● Test suite failed to run

    Jest encountered an unexpected token

    Jest failed to parse a file. This happens e.g. when your code or its dependencies use non-standard JavaScript syntax, or when Jest is not configured to support such syntax.

    Out of the box Jest supports Babel, which will be used to transform your files into valid JS based on your Babel configuration.

    By default "node_modules" folder is ignored by transformers.

    Here's what you can do:
     • If you are trying to use ECMAScript Modules, see https://jestjs.io/docs/ecmascript-modules for how to enable it.
     • If you are trying to use TypeScript, see https://jestjs.io/docs/getting-started#using-typescript
     • To have some of your "node_modules" files transformed, you can specify a custom "transformIgnorePatterns" in your config.
     • If you need a custom transformation specify a "transform" option in your config.
     • If you simply want to mock your non-JS modules (e.g. binary assets) you can stub them out with the "moduleNameMapper" config option.

    You'll find more details and examples of these config options in the docs:
    https://jestjs.io/docs/configuration
    For information about custom transformations, see:
    https://jestjs.io/docs/code-transformation

    Details:

    /private/var/tmp/_bazel_ericharmeling/9a048edd47028dfa357cd8c58b62045b/sandbox/darwin-sandbox/7373/execroot/com_github_cockroachdb_cockroach/bazel-out/darwin-fastbuild/bin/pkg/ui/workspaces/cluster-ui/jest.sh.runfiles/com_github_cockroachdb_cockroach/pkg/ui/workspaces/cluster-ui/node_modules/@cockroachlabs/design-tokens/dist/web/tokens.js:6
    export const ColorBackgroundBase = "#ffffff";
    ^^^^^^

    SyntaxError: Unexpected token 'export'

       9 | // licenses/APL.txt.
      10 | import { cockroach } from "@cockroachlabs/crdb-protobuf-client";
    > 11 | import { Line } from "rc-progress";
         |                         ^
      12 | import React from "react";
      13 | import { Badge } from "src/badge";
      14 |

      at Runtime.createScriptFromCode (../../../../node_modules/jest-runtime/build/index.js:1728:14)
      at Object.<anonymous> (src/jobs/util/progressBar.tsx:11:25)

FAIL src/jobs/jobsPage/jobTable.spec.tsx
  ● Test suite failed to run

    Jest encountered an unexpected token

    Jest failed to parse a file. This happens e.g. when your code or its dependencies use non-standard JavaScript syntax, or when Jest is not configured to support such syntax.

    Out of the box Jest supports Babel, which will be used to transform your files into valid JS based on your Babel configuration.

    By default "node_modules" folder is ignored by transformers.

    Here's what you can do:
     • If you are trying to use ECMAScript Modules, see https://jestjs.io/docs/ecmascript-modules for how to enable it.
     • If you are trying to use TypeScript, see https://jestjs.io/docs/getting-started#using-typescript
     • To have some of your "node_modules" files transformed, you can specify a custom "transformIgnorePatterns" in your config.
     • If you need a custom transformation specify a "transform" option in your config.
     • If you simply want to mock your non-JS modules (e.g. binary assets) you can stub them out with the "moduleNameMapper" config option.

    You'll find more details and examples of these config options in the docs:
    https://jestjs.io/docs/configuration
    For information about custom transformations, see:
    https://jestjs.io/docs/code-transformation

    Details:

    /private/var/tmp/_bazel_ericharmeling/9a048edd47028dfa357cd8c58b62045b/sandbox/darwin-sandbox/7373/execroot/com_github_cockroachdb_cockroach/bazel-out/darwin-fastbuild/bin/pkg/ui/workspaces/cluster-ui/jest.sh.runfiles/com_github_cockroachdb_cockroach/pkg/ui/workspaces/cluster-ui/node_modules/@cockroachlabs/design-tokens/dist/web/tokens.js:6
    export const ColorBackgroundBase = "#ffffff";
    ^^^^^^

    SyntaxError: Unexpected token 'export'

       9 | // licenses/APL.txt.
      10 | import { cockroach } from "@cockroachlabs/crdb-protobuf-client";
    > 11 | import { Line } from "rc-progress";
         |                         ^
      12 | import React from "react";
      13 | import { Badge } from "src/badge";
      14 |

      at Runtime.createScriptFromCode (../../../../node_modules/jest-runtime/build/index.js:1728:14)
      at Object.<anonymous> (src/jobs/util/progressBar.tsx:11:25)


Test Suites: 2 failed, 31 passed, 33 total
Tests:       146 passed, 146 total
Snapshots:   0 total
Time:        268.749 s
Ran all test suites.

It might require some updates to the ui repo and possibly to the jest configuration. I'm going to just hardcode the color for now, since this is the only .tsx file in cluster-ui that uses design-tokens.

@ericharmeling ericharmeling force-pushed the jobs-page-cluster-ui branch 3 times, most recently from d39a7a6 to 5b0957f Compare June 22, 2022 20:03
@ericharmeling ericharmeling force-pushed the jobs-page-cluster-ui branch from 5b0957f to a1813c4 Compare June 22, 2022 21:46
@ericharmeling
Copy link
Contributor Author

bors r+

@craig
Copy link
Contributor

craig bot commented Jun 23, 2022

Build succeeded:

@craig craig bot merged commit 1140872 into cockroachdb:master Jun 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants