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

Add graph view to project overview workloads tab #6901

Merged

Conversation

jeff-phillips-18
Copy link
Member

@jeff-phillips-18 jeff-phillips-18 commented Oct 12, 2020

Fixes
https://issues.redhat.com/browse/ODC-4925

Description
Adds the ability to switch to the graph view in the workloads tab for the project overview page.

Screen Shots:
image

image

Browser conformance:

  • Chrome
  • Firefox
  • Safari
  • Edge

@bgliwa01 @bmignano @serenamarie125

@openshift-ci-robot openshift-ci-robot added the component/core Related to console core functionality label Oct 12, 2020
@openshift-ci-robot openshift-ci-robot added the component/dev-console Related to dev-console label Oct 12, 2020
@serenamarie125
Copy link
Contributor

FYI @alimobrem @beanh66

Copy link

@bgliwa01 bgliwa01 left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Contributor

@serenamarie125 serenamarie125 left a comment

Choose a reason for hiding this comment

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

@jeff-phillips-18 just to be sure, I'm going to include this as a Request for Change

If you are in the Project->Workloads(graph) tab and do an in-context add, you should be brought to a form, and then brought back to the Project->Workloads(graph) tab to see the results. I'm guessing right now it may navigate directly to the Topology page we use in Dev Perspective.

@jeff-phillips-18
Copy link
Member Author

@serenamarie125 currently it takes you back to the workloads tab but with the list view selected :(

Copy link
Contributor

@sahil143 sahil143 left a comment

Choose a reason for hiding this comment

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

@jeff-phillips-18 Creating any resource from the catalog (operator backed, templates) using the Add to Project context menu seems to redirect to the respective Admin page. This is happening because we have checks to redirect in the form based on the active perspective. Is this expected behavior?
Screenshot 2020-10-13 at 7 29 59 PM
Screenshot 2020-10-13 at 7 30 47 PM

@jeff-phillips-18
Copy link
Member Author

@serenamarie125 fixed to redirect to the previous view for the workloads tab.

@debsmita1
Copy link
Contributor

@jeff-phillips-18 In the Project workloads page, I tried adding a database using the Add to Project context menu, on creation it took me to the Template Instance Details page. I was expecting that it would bring me back to the view from where I started

@@ -125,11 +113,52 @@ export const TopologyPageContext: React.FC<TopologyPageProps> = observer(({ matc
);
});

export const TopologyPage: React.FC<TopologyPageProps> = ({ match }) => {
type TopologyWorkloadsPageProps = {
match: any;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
match: any;
match: RMatch<{
name?: string;
}>;

import { useParams } from 'react-router-dom';
import DataModelProvider from '@console/dev-console/src/components/topology/data-transforms/DataModelProvider';
import { TopologyDataRenderer } from '@console/dev-console/src/components/topology/TopologyDataRenderer';
import TopologyPage from '@console/dev-console/src/components/topology/TopologyPage';
Copy link
Member

Choose a reason for hiding this comment

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

This import looks bad. We shouldn't import the dev-console package in public.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is known and already existing. I believe the plan is move Topology out of dev-console and into its own mono repo.

Copy link
Contributor

Choose a reason for hiding this comment

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

If we are to share topology with admin console, we need to move it to a new package as a standalone tool.

edit: I'm not suggesting we do this now :)

</DataModelProvider>
);
type OverviewListPageProps = {
match: any;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
match: any;
match: RMatch<{
ns?: string;
}>;

@vikram-raj
Copy link
Member

There is no padding around the container in the loading state.
Screenshot 2020-10-21 at 2 56 02 PM

@jeff-phillips-18
Copy link
Member Author

@vikram-raj Updated PTAL.

Comment on lines 41 to 34
const setTopologyWorkloadsActiveView = (id: string) => {
localStorage.setItem(LAST_TOPOLOGY_WORKLOADS_VIEW_LOCAL_STORAGE_KEY, id);
};

const getTopologyWorkloadsActiveView = () => {
return localStorage.getItem(LAST_TOPOLOGY_WORKLOADS_VIEW_LOCAL_STORAGE_KEY);
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Does UX want a separate setting for the topology page and for the workload page?

Copy link
Member Author

Choose a reason for hiding this comment

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

UX and PM agreed to using a single setting.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, missed the thought behind this. We are keeping view preference separate per sections (Workloads vs Topology). UX and PM agreed to that. The saved layout of the graph view is shared between the sections.

Comment on lines 159 to 163
{workloadsPage ? (
<TopologyWorkloadsPage match={match} />
) : (
<TopologyPageContext match={match} />
)}
Copy link
Contributor

Choose a reason for hiding this comment

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

Both views are exactly the same minus the project selector.
I think they should share a lot more in common between the two pages and come up with a common strategy for the URL handling.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated.

Comment on lines 61 to 68
const next =
activePerspective === 'dev'
? '/topology'
: `${resourcePathFromModel(
ClusterServiceVersionModel,
match.params.appName,
match.params.ns,
)}/${match.params.plural}`;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we can change the location we go to after creating an operand. The user who was in the installed operators nav item will now be redirected to the workloads tab even though an operand may not result in a workload being created.

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed this change

@jeff-phillips-18 jeff-phillips-18 force-pushed the workloads-graph branch 3 times, most recently from 20bc818 to 9498468 Compare October 27, 2020 14:58
@serenamarie125
Copy link
Contributor

@alimobrem @beanh66 FYI

@jeff-phillips-18
Copy link
Member Author

/retest

Copy link
Member

@jerolimov jerolimov left a comment

Choose a reason for hiding this comment

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

I tested this a while and it works great for me.

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 29, 2020
@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 3, 2020
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

16 similar comments
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Nov 4, 2020
@jeff-phillips-18
Copy link
Member Author

@jerolimov @christianvogt Updated TopologyPage to fix tests

@christianvogt
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 4, 2020
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bgliwa01, christianvogt, jeff-phillips-18, jerolimov

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

@openshift-merge-robot openshift-merge-robot merged commit 1ae6aea into openshift:master Nov 5, 2020
@spadgett spadgett added this to the v4.7 milestone Nov 5, 2020
@jeff-phillips-18 jeff-phillips-18 deleted the workloads-graph branch December 2, 2020 13:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. component/core Related to console core functionality component/dev-console Related to dev-console component/olm Related to OLM lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet