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 Trigger and Subscription modal in topology #6080

Merged

Conversation

karthikjeeyar
Copy link
Contributor

@karthikjeeyar karthikjeeyar commented Jul 22, 2020

Fixes: https://issues.redhat.com/browse/ODC-4246 , https://issues.redhat.com/browse/ODC-4167

Problem:
Ability to add trigger/subscription from topology gestures and kebab actions

Solution:
Added Trigger modal and kebab actions for triggers.
Added Subscription modal and kebab actions for subscriptions
Added context menu on the resource backed edges

Note: Sidebar enhancements will be added in a follow up PR

Screenshot:
Trigger Modal:

image

Subscription Modal:

image

Empty state:

image

Error state:
image

Gif:
brokers-triggers-modal

cc: @rachael-phillips @beaumorley @openshift/team-devconsole-ux

@karthikjeeyar
Copy link
Contributor Author

/assign @invincibleJai

@karthikjeeyar
Copy link
Contributor Author

/retest

@serenamarie125
Copy link
Contributor

@rachael-phillips can you confirm? I think that the label on the Add Trigger modal should be "Subscriber", not "Select Subscriber"

@serenamarie125
Copy link
Contributor

Can we have the Attribute/Value be the same width as the other input widgets? Right now there's extra space between the delete icon & the edge of the modal.

@karthikjeeyar karthikjeeyar force-pushed the feat-knative-actions branch 3 times, most recently from 297b13c to 921c1d0 Compare July 23, 2020 05:01
@karthikjeeyar
Copy link
Contributor Author

karthikjeeyar commented Jul 23, 2020

@rachael-phillips can you confirm? I think that the label on the Add Trigger modal should be "Subscriber", not "Select Subscriber"

@serenamarie125 @rachael-phillips Like we have in Design doc, I have changed the title to Subscriber and updated the screenshots. The filter attributes section in the trigger modal uses the nameValueEditor field that we use every where in console has this type of spacing.
eg: ApiServersource, sinkbinding event source forms

image

cc: @christianvogt

@karthikjeeyar karthikjeeyar force-pushed the feat-knative-actions branch 5 times, most recently from 51d1ca8 to 877ab26 Compare July 23, 2020 08:55
@rachael-phillips
Copy link

rachael-phillips commented Jul 23, 2020

Hey @karthikjeeyar @serenamarie125 I can confirm that the recommendation to have Subscriber rather than Select Subscriber is correct. I think we should eventually update the spacing of the ATTRIBUTE, KEY, VALUE, APIVERSION, KIND, etc. form fields across the flow, but agree with keeping it standard for now rather than updating based on what I've seen in other places. For example, this is another modal on the topology view, and for consistency I think we should go with the standard spacing. To be super clear, I think what Karthik has now for spacing these form fields is correct.
Screen Shot 2020-07-23 at 9 50 05 AM

@serenamarie125 I will add your recommendation to this Audit I am currently working on of our form fields so we can address this in the future: https://docs.google.com/document/d/10QMFBrCGmwbq92Vy8GSNteAX6EgslTcR4PnMoQwF_1w/edit?usp=sharing

@christianvogt
Copy link
Contributor

@karthikjeeyar ran into an issue where if I set only the value of a filter and then submit the form, I get an error and then the filters key, value, and Add button go missing.

image

@christianvogt
Copy link
Contributor

The rest seems to function well.

@rachael-phillips
Copy link

LGTM!

@invincibleJai
Copy link
Member

/lgtm
/approve

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: invincibleJai, karthikjeeyar

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-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 28, 2020
@invincibleJai
Copy link
Member

Verified , works as expected!!

@karthikjeeyar
Copy link
Contributor Author

/retest

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-merge-robot openshift-merge-robot merged commit e36382a into openshift:master Jul 28, 2020
@spadgett spadgett added this to the v4.6 milestone Jul 30, 2020
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/dev-console Related to dev-console component/knative Related to knative-plugin 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

9 participants