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

feat add support to move sink for eventSource #4847

Merged

Conversation

invincibleJai
Copy link
Member

@invincibleJai invincibleJai commented Mar 30, 2020

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

Analysis / Root cause:
User can't change/move Sink for event source through web console

Solution Description:
User can change/move Sink for event source through web console through context menu/action item

Screen shots / Gifs for design review:
Mar-31-2020 15-39-04

Screenshot 2020-03-31 at 5 15 46 PM

cc @serenamarie125

Unit test coverage report:
Screenshot 2020-03-31 at 5 05 51 PM
Screenshot 2020-04-02 at 4 37 29 PM

Browser conformance:

  • Chrome
  • Firefox
  • Safari
  • Edge

@openshift-ci-robot openshift-ci-robot added the component/knative Related to knative-plugin label Mar 30, 2020
@openshift-ci-robot openshift-ci-robot added the component/shared Related to console-shared label Mar 30, 2020
@invincibleJai invincibleJai changed the title fead add support tp mpve sink for eventSource feat add support tp move sink for eventSource Mar 30, 2020
@invincibleJai invincibleJai changed the title feat add support tp move sink for eventSource feat add support to move sink for eventSource Mar 30, 2020
@invincibleJai invincibleJai changed the title feat add support to move sink for eventSource [WIP] feat add support to move sink for eventSource Mar 30, 2020
@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 30, 2020
@invincibleJai invincibleJai changed the title [WIP] feat add support to move sink for eventSource feat add support to move sink for eventSource Mar 31, 2020
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 31, 2020
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.

Looks great @invincibleJai ! LGTM

@andrewballantyne
Copy link
Contributor

/kind feature

@openshift-ci-robot openshift-ci-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Mar 31, 2020
},
},
};
const formProps: SinkSourceModalProps = {
Copy link
Contributor

Choose a reason for hiding this comment

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

make this common so the other modal component unit test can use this? WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

i was wondering if we could reuse this formik properities in all other formik wrapped components but not sure if we can keep it as separate utility for reusing.

Copy link
Member Author

Choose a reason for hiding this comment

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

@sahil143 @karthikjeeyar we can reuse for modal component , any suggested file/location to place it as it could be in different packages as well

Copy link
Member Author

Choose a reason for hiding this comment

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

have moved it to shared 24331a6

<ModalBody>
<p>
Select a sink to move the event source
<strong>{` ${resourceName} `}</strong>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<strong>{` ${resourceName} `}</strong>
<strong>{resourceName}</strong>

Copy link
Member Author

Choose a reason for hiding this comment

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

@sahil143 used <strong>{ ${resourceName} }</strong> to add space i.e

Screenshot 2020-04-02 at 12 12 30 PM

Copy link
Contributor

Choose a reason for hiding this comment

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

this would also work 😉

<strong> {resourceName} </strong>

Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't this work?
Select a sink to move the event source <strong>{resourceName}</strong> to

Copy link
Member Author

Choose a reason for hiding this comment

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

let me try it

Copy link
Member Author

Choose a reason for hiding this comment

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

It's working , not sure what i was thinking 🤔

},
},
};
const formProps: SinkSourceModalProps = {
Copy link
Contributor

Choose a reason for hiding this comment

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

i was wondering if we could reuse this formik properities in all other formik wrapped components but not sure if we can keep it as separate utility for reusing.

@invincibleJai invincibleJai force-pushed the feat-eventsrc-sink-modal branch 2 times, most recently from a9528c4 to 24331a6 Compare April 2, 2020 08:43
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.

/lgtm

verified changes locally

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 2, 2020
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Apr 2, 2020
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.

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 2, 2020
<ModalBody>
<p>
Select a sink to move the event source
<strong>{` ${resourceName} `}</strong>
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't this work?
Select a sink to move the event source <strong>{resourceName}</strong> to

import SinkSource from './SinkSource';

type SinkSourceControllerProps = {
obj: K8sResourceKind;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we be more specific on the prop name. Why not source? I see this pattern is copied from TrafficSplittingController ;/

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 :)

@christianvogt
Copy link
Contributor

@invincibleJai I also made this comment but it seems to be buried in the thread somewhere:
https://github.com/openshift/console/pull/4847/files#r402471794

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Apr 2, 2020
@christianvogt
Copy link
Contributor

/lgtm
/approve

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: christianvogt, invincibleJai, sahil143, serenamarie125

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 Apr 2, 2020
@openshift-merge-robot openshift-merge-robot merged commit b0c09f3 into openshift:master Apr 2, 2020
@spadgett spadgett added this to the v4.5 milestone Apr 7, 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/knative Related to knative-plugin component/shared Related to console-shared kind/feature Categorizes issue or PR as related to a new feature. 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