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

Archive runs UI #748

Merged
merged 29 commits into from
Feb 13, 2019
Merged

Archive runs UI #748

merged 29 commits into from
Feb 13, 2019

Conversation

yebrahim
Copy link
Contributor

@yebrahim yebrahim commented Jan 29, 2019

This kicks off the archive UI experience. It only supports runs now, but more resources might be added to it in the future. The interaction is simple: select any number of runs, click archive, and confirm. You can then view the list of archived runs in the archive view, which is visible on the side nav.

image

Included in this PR (in file order):

  • Mock backend code is cleaned up to remove options handlers, which are no longer needed with the React middleware.
  • Added archive/unarchive support in mock backend, as well as filtering on storage state.
  • New archive page, along with router and side nav changes.
  • When listing archived/unarchived resources, a resource with no value in the storage state field is treated as available, to account for back compat, and if for some reason the field is removed manually.
  • Cleaned up ExperimentDetails buttons, which were still hard-coded, to use the new Buttons module.
  • Cleaned up tests verbose log noise.

Things to improve that this PR does not tackle:

  • All "New run" buttons now have the same tooltip. Previously, the tooltip would indicate whether the run is created out of a pipeline, or in the current experiment.
  • The breadcrumbs on archived resources aren't changed, so they don't indicate that the user is looking at an archived resource. This will probably require more plumbing changes to the way we do breadcrumbing, and I think should be tackled separately.

Fixes #621.


This change is Reviewable

@vicaire vicaire removed their request for review January 30, 2019 21:42
@yebrahim yebrahim changed the title Ui archive Archive runs UI Feb 1, 2019
@ajayalfred
Copy link
Contributor

I'm not seeing the 'Archive' action on the run details page. Is there a reason it's not included on that page?

UX changes:

  1. Archive is the last section in the left nav above the collapse button. Separated from the above section by a pin line and from the collapse button by a pin line.
    image

  2. Dialog and text for archiving a run / runs below. Note affirmative action (Archive / Restore) must be to the right of Cancel.
    image

  3. Dialog and text for restoring a run / runs below.
    image

@ajayalfred
Copy link
Contributor

Archive on run details page looks great. 2 pieces of feedback

  1. Please move the affirmative action to the right of Cancel in dialogs. Archive or Restore action to the right of Cancel
    image

  2. Known issue and already being tracked – breadcrumb path of archived runs.

@yebrahim
Copy link
Contributor Author

yebrahim commented Feb 7, 2019

Done, PTAL. I manually tweaked the breadcrumbs in the run details page to handle showing the archive piece. I think there's an opportunity to refactor this and clean it up, but since this is the only breadcrumbs exception I think it's fine.

@yebrahim
Copy link
Contributor Author

yebrahim commented Feb 7, 2019

/test kubeflow-pipeline-build-image

1 similar comment
@rileyjbauer
Copy link
Contributor

/test kubeflow-pipeline-build-image

}
const pageTitle = <div className={commonCss.flex}>
{statusToIcon(runMetadata.status as NodePhase, runDetail.run!.created_at)}
<span style={{ marginLeft: 10 }}>{runMetadata.name!}</span>
</div>;

this.props.updateToolbar({ breadcrumbs, pageTitle, pageTitleTooltip: runMetadata.name });
const buttons = new Buttons(this.props, this.refresh.bind(this));
Copy link
Contributor

Choose a reason for hiding this comment

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

is it okay to create a new bound function here every time the page refreshes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean in terms of performance? It'd be slightly better to bind it once on the class, it's not really concerning if this code doesn't get called very frequently. If it's inside the render method, for example, it would be a little more important, but even then, there's no reason to do perf-tweaking for the run details page.

@rileyjbauer
Copy link
Contributor

/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rileyjbauer

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

1 similar comment
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rileyjbauer

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@rileyjbauer
Copy link
Contributor

/test kubeflow-pipeline-e2e-test

@k8s-ci-robot k8s-ci-robot merged commit 603597d into kubeflow:master Feb 13, 2019
@yebrahim yebrahim deleted the ui-archive branch March 6, 2019 19:06
magdalenakuhn17 pushed a commit to magdalenakuhn17/pipelines that referenced this pull request Oct 22, 2023
* add the new gateway

* remove the kfserving from the install docs

* make ingress gateway a variable

* make ingress gateway a variable

* revert all to istio-ingress

* add a note for kfserving gateway
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants