Skip to content

Add group selection to developer topology view #2802

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.

Sign up for GitHub

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

Jump to bottom
Merged
openshift-merge-robot merged 1 commit into from
Sep 30, 2019
Merged

Add group selection to developer topology view #2802

openshift-merge-robot merged 1 commit into from
Sep 30, 2019

Conversation

@jeff-phillips-18
Copy link
Member

@jeff-phillips-18 jeff-phillips-18 commented Sep 20, 2019

@openshift-ci-robot openshift-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Sep 20, 2019
@openshift-ci-robot openshift-ci-robot added the component/dev-console Related to dev-console label Sep 20, 2019
divyanshiGupta
divyanshiGupta reviewed Sep 25, 2019
View reviewed changes
frontend/packages/dev-console/src/components/topology/TopologyResourcePanel.tsx Outdated
* Datacontroller get the buildConfigs based on apps.kubernetes.io/instance label which is not applied to apps created using browser catalog
*/

function metadataUIDCheck(items: any): ResourceProps[] {
Copy link
Contributor

@divyanshiGupta divyanshiGupta Sep 25, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

nit: can you add a type here?

divyanshiGupta
divyanshiGupta reviewed Sep 25, 2019
View reviewed changes
frontend/packages/dev-console/src/components/topology/shapes/DefaultGroup.tsx Outdated
const { nodes, lowestPoint, containerPath } = this.state;

if (nodes.length === 0) {
return null;
}

const pathClasses = classNames('odc-default-group', { 'is-highlight': dropTarget });
const handleClick = (e) => {
Copy link
Contributor

@divyanshiGupta divyanshiGupta Sep 25, 2019

Choose a reason for hiding this comment

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

nit: can you add event type here?

@divyanshiGupta
Copy link
Contributor

divyanshiGupta commented Sep 25, 2019

@jeff-phillips-18 for this story are we only going to show the name of the application group in the sidebar? Also the title in the design doc has a resource badge in front of the name and the title color is also different.

This is how it looks right now
Screenshot from 2019-09-25 21-40-34

Design doc
Screenshot from 2019-09-25 21-40-05

Also, there is a blue border around the group. Are we going to add that ?

@jeff-phillips-18
Copy link
Member Author

jeff-phillips-18 commented Sep 25, 2019

I am updating the styles per the design doc now. I had a few other questions on some of the styling as well.

I don't believe we have the application badge yet but I will confirm.

@jeff-phillips-18 jeff-phillips-18 changed the base branch from master-4.3 to master September 25, 2019 20:48
@openshift-ci-robot openshift-ci-robot added the component/core Related to console core functionality label Sep 25, 2019
@jeff-phillips-18
Copy link
Member Author

jeff-phillips-18 commented Sep 25, 2019

Added styling and application badge.

@jeff-phillips-18 jeff-phillips-18 force-pushed the group-select branch 2 times, most recently from fbcdaa4 to 3041616 Compare September 27, 2019 10:51
@divyanshiGupta
Copy link
Contributor

divyanshiGupta commented Sep 27, 2019

@jeff-phillips-18 Code looks good to me. Also I tried it locally, works fine.

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 27, 2019
@divyanshiGupta
Copy link
Contributor

divyanshiGupta commented Sep 27, 2019

@jeff-phillips-18 noticed one thing that the color of the title is still black and not blue as presented in the design doc. Are we going to keep it as black?

@jeff-phillips-18
Copy link
Member Author

jeff-phillips-18 commented Sep 27, 2019

@divyanshiGupta It is blue in the doc because eventually it should be a link (when applications are actual objects with a list page and such). That doesn't exist yet so it should be black until it becomes a link.

@divyanshiGupta
Copy link
Contributor

divyanshiGupta commented Sep 27, 2019

@divyanshiGupta It is blue in the doc because eventually it should be a link (when applications are actual objects with a list page and such). That doesn't exist yet so it should be black until it becomes a link.

Yes, agreed.

@jeff-phillips-18
Copy link
Member Author

jeff-phillips-18 commented Sep 27, 2019

application-selection

@serenamarie125 @Veethika

@serenamarie125
Copy link
Contributor

serenamarie125 commented Sep 30, 2019

@jeff-phillips-18 questions on badge. Is there a reason we are using APP rather than A? And how did you pick the color? It probably should be a unique badge color.

@jeff-phillips-18
Copy link
Member Author

jeff-phillips-18 commented Sep 30, 2019

The default for badges is the first 4 characters of the kind. I can update but I chose to start with the default. The color is unique, I chose one close to what was in the mocks that was a PF3 pallet color (since all the badges are PF3 pallet colors).

@serenamarie125
Copy link
Contributor

serenamarie125 commented Sep 30, 2019

ok, that's great about the unique color, thanks @jeff-phillips-18 !

@serenamarie125
Copy link
Contributor

serenamarie125 commented Sep 30, 2019

I'd suggest just utilizing "A" for Application unless others object @jeff-phillips-18

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

jeff-phillips-18 commented Sep 30, 2019

Updated to use A as the badge.

@jeff-phillips-18
Copy link
Member Author

jeff-phillips-18 commented Sep 30, 2019

/retest

@christianvogt
Copy link
Contributor

christianvogt commented Sep 30, 2019

/lgtm
/approve

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 30, 2019
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Sep 30, 2019

[APPROVALNOTIFIER] This PR is APPROVED

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

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

The pull request process is described here

Details 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-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 30, 2019
@spadgett
Copy link
Member

spadgett commented Sep 30, 2019

/hold
for #2865 (sorry)

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 30, 2019
@spadgett
Copy link
Member

spadgett commented Sep 30, 2019

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 30, 2019
@openshift-merge-robot openshift-merge-robot merged commit 8334a4b into openshift:master Sep 30, 2019
@spadgett spadgett added this to the v4.3 milestone Oct 4, 2019
@jeff-phillips-18 jeff-phillips-18 deleted the group-select branch November 19, 2019 11:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@andrewballantyne andrewballantyne Awaiting requested review from andrewballantyne

@christianvogt christianvogt Awaiting requested review from christianvogt

@sahil143 sahil143 Awaiting requested review from sahil143

1 more reviewer

@divyanshiGupta divyanshiGupta divyanshiGupta left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@christianvogt christianvogt

@divyanshiGupta divyanshiGupta

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 lgtm Indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.

Projects

None yet

Milestone

v4.3

Development

Successfully merging this pull request may close these issues.

7 participants

@jeff-phillips-18 @divyanshiGupta @serenamarie125 @christianvogt @openshift-ci-robot @spadgett @openshift-merge-robot