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.

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

add Channels, EventSources, OperatorBacked, HelmCharts menu action to Add To Project context menu #6838

Merged
merged 2 commits into from Oct 14, 2020
Merged

add Channels, EventSources, OperatorBacked, HelmCharts menu action to Add To Project context menu #6838

merged 2 commits into from Oct 14, 2020

Conversation

sahil143
Copy link
Contributor

@sahil143 sahil143 commented Oct 5, 2020 •
edited

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

Analysis / Root cause: missing Channels, EventSources, OperatorBacked, HelmCharts from Add to Project sub menu

Solution Description: added Channels, EventSources, OperatorBacked, HelmCharts menu action

Screen shots / Gifs for design review:
Screenshot 2020-10-05 at 9 35 11 PM
image

Unit test coverage report:

Test setup:

Browser conformance:

  • Chrome
  • Firefox
  • Safari
  • Edge

@openshift-ci-robot openshift-ci-robot added the component/dev-console Related to dev-console label Oct 5, 2020
@openshift-ci-robot openshift-ci-robot added component/knative Related to knative-plugin component/shared Related to console-shared labels Oct 5, 2020
@andrewballantyne
Copy link
Contributor

/cc @rohitkrai03 @christianvogt

I'm seeing a lack of extensibility here, and Helm stuff showing up in shared. Need Rohit / Christian to chime in here with suggestions.

@christianvogt
Copy link
Contributor

I'd like to keep helm out of shared. It will become it's own package soon.
Until we improve the actions extension 4.7, I'm ok with the hard coding as it's already done for the other actions on the graph.

@invincibleJai
Copy link
Member

/assign

@invincibleJai
Copy link
Member

invincibleJai commented Oct 7, 2020 •
edited

@sahil143 one issue i see but this is not with this PR however as we have more options now so it's easily reproduced, if we try to add to project from lower quadrant of screen, we may need to track this as separate issue

Oct-07-2020 17-40-00

@sahil143
Copy link
Contributor Author

sahil143 commented Oct 8, 2020

/kind feature

@openshift-ci-robot openshift-ci-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Oct 8, 2020
@bgliwa01
Copy link

bgliwa01 commented Oct 8, 2020

will there be a separate PR for adding Operator Backed to an existing component?

@sahil143
Copy link
Contributor Author

sahil143 commented Oct 8, 2020

will there be a separate PR for adding Operator Backed to an existing component?

@bgliwa01 Do you mean through connector? If yes, that will be another story. This PR only takes care of adding action items to the context menu.

@sahil143
Copy link
Contributor Author

sahil143 commented Oct 8, 2020

@sahil143 one issue i see but this is not with this PR however as we have more options now so it's easily reproduced, if we try to add to project from lower quadrant of screen, we may need to track this as separate issue

@invincibleJai I could reproduce it on the patternfly-react repo react-topology package itself.. So it will be fixed there. I have raised and issue to track this https://issues.redhat.com/browse/ODC-4988

react-topology demo app screenshot
Screenshot 2020-10-08 at 9 51 48 PM

bgliwa01
bgliwa01 approved these changes Oct 9, 2020
View reviewed changes
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

invincibleJai
invincibleJai reviewed Oct 9, 2020
View reviewed changes
frontend/packages/dev-console/src/components/helm/HelmChartsIcons.tsx Outdated
<img className={className} style={style} src={helmIcon} alt="Helm Charts Logo" />
);

export default HelmChartsIcon;
Copy link
Member

Choose a reason for hiding this comment

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

nit: shall we have file name as well HelmChartsIcon

frontend/packages/knative-plugin/src/actions/add-channel.tsx Outdated
Comment on lines 13 to 15
const EventChannelIcon: React.FC = () => {
return <img style={eventChannelStyles} src={channelIcon} alt="" />;
};
Copy link
Member

Choose a reason for hiding this comment

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

nit: can use implicit return here

@sahil143
remove HelmChartIcon from the shared package and add unit tests
cc76921 
change HelmChartIcon name and use implicit return
karthikjeeyar
karthikjeeyar approved these changes Oct 13, 2020
View reviewed changes
Copy link
Contributor

@karthikjeeyar karthikjeeyar left a comment

Choose a reason for hiding this comment

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

Verified locally, works fine
/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 13, 2020
@invincibleJai
Copy link
Member

/approve

Verified, works as expected.

@invincibleJai
Copy link
Member

/approve

@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

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

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 Oct 14, 2020
@openshift-merge-robot openshift-merge-robot merged commit 97f1b62 into openshift:master Oct 14, 2020
@spadgett spadgett added this to the v4.7 milestone Oct 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@invincibleJai invincibleJai invincibleJai left review comments

@karthikjeeyar karthikjeeyar karthikjeeyar approved these changes

@bgliwa01 bgliwa01 bgliwa01 approved these changes

@debsmita1 debsmita1 Awaiting requested review from debsmita1

@TheRealJon TheRealJon Awaiting requested review from TheRealJon

@christianvogt christianvogt Awaiting requested review from christianvogt

@rohitkrai03 rohitkrai03 Awaiting requested review from rohitkrai03

Assignees

@invincibleJai invincibleJai

@karthikjeeyar karthikjeeyar

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 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
Milestone
v4.7
Development

Successfully merging this pull request may close these issues.

None yet
9 participants
@sahil143 @andrewballantyne @christianvogt @invincibleJai @bgliwa01 @openshift-ci-robot @karthikjeeyar @spadgett @openshift-merge-robot